wallet: List all wallets in non-SQLite and non-BDB builds #20275
Conversation
|
Concept ACK. In 6066221 commit message:
What are those dependencies? |
Oh, I think I need to drop that comment. The dependency I was thinking of was |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
@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. |
|
Concept ACK |
|
ACK e21dc6c |
|
Tested ACK e21dc6c. Tested Startup fails if any wallets |
|
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" |
The same error would also happen if the the user attempts to open sqlite wallet with previous versions. |
|
Also, it would be hard to create a sqlite wallet with a non-sqlite build. Assigning 22.0 milestone |
|
Rebased e21dc6c -> 417e95a ( |
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
No change to behavior. This is just cleanup after previous MOVEONLY commit to make db.h list function fit conventions of surrounding functions.
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.
…ions 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.
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.
|
Rebased 727e0aa -> f3d870f ( |
|
Concept ACK |
|
ACK f3d870f |
|
Tested ACK f3d870f. Tested a --without-sqlite build with sqlite wallets.
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? |
|
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. |
|
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. |
|
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 bitcoin-core/gui#95 (comment) 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. |
Yes, that's what I ACK'ed above. Ah I keep forgetting bitcoin-core/gui#95 😩 thanks for pointing it. |
|
Thanks for testing and clarifying! Sorry I didn't understand right away. |
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.