Advantages of this change:
GetWalletDir in multiple places;The WalletLocation class can be a way to represent a wallet not yet loaded that exists in the wallet directory.
You accidentally removed a circular dependency:
0Good job! The circular dependency "wallet/rpcwallet -> wallet/wallet -> wallet/rpcwallet" is no longer present.
Concept ACK
Nice bonus with the removed circular dependency!
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, 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.
27+ , m_external(other.m_external)
28+ , m_name(other.m_name)
29+ , m_path(other.m_path) {}
30+
31+ //! Returns the wallet filename.
32+ std::string GetFilename() const { return m_filename; };
3828@@ -3830,23 +3829,23 @@ bool CWallet::Verify(std::string wallet_file, bool salvage_wallet, std::string&
3829 // 3. Path to a symlink to a directory.
3830 // 4. For backwards compatibility, the name of a data file in -walletdir.
3831 LOCK(cs_wallets);
3832- fs::path wallet_path = fs::absolute(wallet_file, GetWalletDir());
3833+ fs::path wallet_path = info.GetPath();
const ref? :-)
39+
40+ //! Get wallet absolute path.
41+ const fs::path& GetPath() const { return m_path; }
42+
43+ //! Return whether the wallet lives in the wallet directory. See ::GetWalletDir().
44+ bool IsExternal() const { return m_external; }
IsExternal() and m_external seem unused?
670@@ -671,7 +671,7 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
671 * Used in debug logs and to send RPCs to the right wallet instance when
This PR is doing two different things:
WalletInfo class to hold information about a wallet’s location.I like the first thing, but don’t think I like or understand the second thing and would prefer to see all of the m_external/IsExternal/"external" stuff moved out of this PR into a separate PR if it is going to be pursued.
I also think WalletInfo should be have a more meaningful name. Maybe WalletLocation?
Thanks @ryanofsky:
WalletInfo comes from QFileInfo, but I agree the name is not clear. WalletLocation is too specific to paths?filesystem heuristic
IIUC wallets inside -walletdir are internal, the others are external?
I hope you will move the external stuff to a separate PR with a justification about why it is useful, so reviewers don’t need to guess and speculate about what it’s for (I don’t have an idea), and so we can talk about it separately from this refactoring stuff.
Also, could you be clearer about what the problem is with WalletLocation? For example, does it imply something about the class which isn’t true? I don’t know what “too specific to paths” means.
For example, does it imply something about the class which isn’t true?
If GetName(), GetDisplayName() and eventually other attributes like IsExternal fit WalletLocation then I’m fine with that.
Took both suggestions from @ryanofsky and rebased.
This is a dependency for full multi wallet support in the UI — there was a lot of duplicate code in #13100.
91+ m_name = name.string();
92+}
93+
94+std::string WalletLocation::GetDisplayName() const
95+{
96+ return m_name.empty() ? "[default wallet]" : m_name;
132@@ -133,7 +133,7 @@ void TestGUI()
133 for (int i = 0; i < 5; ++i) {
134 test.CreateAndProcessBlock({}, GetScriptForRawPubKey(test.coinbaseKey.GetPubKey()));
135 }
136- std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>("mock", WalletDatabase::CreateMock());
137+ std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>(WalletLocation("mock"), WalletDatabase::CreateMock());
74@@ -75,3 +75,29 @@ std::vector<fs::path> ListWalletDir()
75
76 return paths;
77 }
78+
79+WalletLocation::WalletLocation(const std::string& filename) : m_filename(filename)
80+{
81+ const fs::path wallet_dir = GetWalletDir();
82+ m_path = fs::absolute(filename, wallet_dir);
83+ fs::path name, parent = m_path;
This code added here is more complicated than it needs to be and it is not a refactoring because it would change behavior, and in bad ways for windows and case insensitive filesystems. If the intention is to change behavior, this should be done in separate, smaller PR and accompanied by tests so it can be better understood and discussed. For a clean refactoring in this PR, this entire function should just be:
0WalletLocation::WalletLocation(const std::string& name) : m_name(name), m_path(fs::absolute(name, GetWalletDir()) {}
And the m_filename and GetFileName() members should be dropped and all GetFileName() calls changed to GetName().
The way -rpcwallet and -wallet arguments were designed to work is that there is no string or path manipulation done when matching wallet names that would ever cause the wrong wallet to be used or the right wallet not to be found.
So for example, if a user starts bitcoind with -wallet=c:\wallets\bob\2018\ they should be able to run bitcoin-cli with the exact same -rpcwallet=c:\wallets\bob\2018\ value, and not have to figure out a different value like bob/2018 or C:\Wallets\Bob\2018 or whatever else the code added in this function might change it to.
I think we could consider changing this behavior, but only if we do it in a clear and deliberate way. I’d definitely want it to be done after this refactoring instead of simultaneously with it.
23+ fs::path m_path;
24+
25+public:
26+ explicit WalletLocation() {}
27+ explicit WalletLocation(const std::string& filename);
28+ explicit WalletLocation(const WalletLocation& other)
35+
36+ //! Returns the wallet display name.
37+ std::string GetDisplayName() const;
38+
39+ //! Get wallet name.
40+ const std::string& GetName() const { return m_name; }
GetName and GetFileName in this class is unnecessarily confusing, and how these functions are called breaks current behavior. I can see why it’d be useful to use canonicalized wallet names in the gui in the future, but I think that functionality should be added separately when it’s actually ready to be used.
The problem I had was when I opened an external wallet (with the file dialog) that was in the wallet dir. Whatever the user chooses it should be resolved.
I think loading a wallet from the gui can basically work the same as calling the loadwallet RPC except if a user browsed for a wallet path, and the path is literally underneath <walletdir>, then the <walletdir> prefix should get stripped from the wallet name. Also, loading the same wallet twice should not be an error. The GUI can handle the “duplicates fileid” condition https://github.com/bitcoin/bitcoin/blob/5c25409d6851182c5e351720cee36812c229b77a/src/wallet/db.cpp#L48 by switching to the already open wallet and maybe showing a notification instead of an error.
Both these things could be be implemented in the GUI with minimal changes to wallet code.
2414- if (fs::symlink_status(wallet_path).type() == fs::file_not_found) {
2415- throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Wallet " + wallet_file + " not found.");
2416- } else if (fs::is_directory(wallet_path)) {
2417+ if (!location.Exists()) {
2418+ throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Wallet " + location.GetName() + " not found.");
2419+ } else if (fs::is_directory(location.GetPath())) {
WalletLocation method to hold directory checking logic below. Maybe WalletLocation::HasData, or WalletLocation::IsEmpty, or something like that. After #14552 (aef19be5851ceb3b324fd80756060a00aad8e61c) this could also use SplitWalletPath.
WalletLocation::GetEnvironmentPath() and WalletLocation::GetDatabasePath()?
Is it bad to add WalletLocation::GetEnvironmentPath() and WalletLocation::GetDatabasePath()?
It’s not terrible, but I think it’d be better not to have berkeley stuff baked into the WalletLocation interface. Maybe a wallet location doesn’t need to be a btree file and environment directory. Maybe it could be some other type of database path or connection string.
I think it would be an improvement if this code were more like:
0if (!location.Exists()) {
1 throw RPCError("Specified wallet path doesn't exist. Call createwallet to create a wallet at this path.");
2} else if (!location.HasData()) {
3 throw RPCError("Not loading wallet. Specified path doesn't does not appear to be a valid wallet.");
4}
But I think more ideally BerkeleyDatabase would have a bool create option that that loadwallet would set false and createwallet would set true, so error checking at this level could be removed entirely, and shared with the wallet tool from #13926, for example.
utACK 65f3672f3b82a6fa30e5171f85bc8d8a29e0797e
It’s not terrible, but I think it’d be better not to have berkeley stuff baked into the WalletLocation interface.
I tend to agree, it’s best to keep it more or less database independent. No matter the rest of the change it should at least not become more difficult to swap for another database.