-
Notifications
You must be signed in to change notification settings - Fork 37.7k
External wallet files #11687
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
External wallet files #11687
Conversation
This is too dangerous. It's not clear at face value to users that the |
FWIW, if you separate a wallet from its database directory you reliably get a wallet that won't open. To see this for yourself, copy a wallet.dat from a running wallet to another directory and try starting another copy of bitcoin against it there. |
Agree with @luke-jr, #11466 is safer until we decide on a better wallet directory structure (e.g. #11466 (comment)), which would involve supporting multiple BDB environments first. |
Note that you might want to cherrypick c36cb54 to help debug the travis failure |
Only if it has uncompacted log files. To prevent this, bitcoin spends so much time consolidating the wallet database at runtime (in the MaybeCompactWalletDB thread), and does it at least at a clean shutdown. People are moving around wallet.dat already, from and to the data directory, so I'm not convinced that supporting multiple wallet directories in principle makes the risk much larger that wallets are separated from their database directory while "live". But indeed, a database environment has to be created where the wallets are. Creating wallets in different places or even on different file systems but having the database environment in the main data directory is asking for trouble. |
src/bitcoin-cli.cpp
Outdated
@@ -48,7 +48,7 @@ std::string HelpMessageCli() | |||
strUsage += HelpMessageOpt("-rpcclienttimeout=<n>", strprintf(_("Timeout in seconds during HTTP requests, or 0 for no timeout. (default: %d)"), DEFAULT_HTTP_CLIENT_TIMEOUT)); | |||
strUsage += HelpMessageOpt("-stdinrpcpass", strprintf(_("Read RPC password from standard input as a single line. When combined with -stdin, the first line from standard input is used for the RPC password."))); | |||
strUsage += HelpMessageOpt("-stdin", _("Read extra arguments from standard input, one per line until EOF/Ctrl-D (recommended for sensitive information such as passphrases). When combined with -stdinrpcpass, the first line from standard input is used for the RPC password.")); | |||
strUsage += HelpMessageOpt("-rpcwallet=<walletname>", _("Send RPC for non-default wallet on RPC server (argument is wallet filename in bitcoind directory, required if bitcoind/-Qt runs with multiple wallets)")); | |||
strUsage += HelpMessageOpt("-rpcwallet=<walletname>", _("Send RPC for non-default wallet on RPC server (needs to exactly match corresponding -wallet option passed to bitcoind)")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Full path? How will the url look like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Full path? How will the url look like?
It doesn't have to be the full path, but it can be. Paths are urlencoded, and symlinks are also allowed. See the unit test which tests every imaginable type of path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels weird to known server side details. Not to mention it can break the client if you move the wallet to a new place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels weird to known server side details. Not to mention it can break the client if you move the wallet to a new place.
The PR is not changing anything about this. Whatever string the server uses to identify the wallet, the client just has to use the same string. We've discussed adding and storing wallet identifiers in the past, but opted to just use filenames for simplicity. Server operator is free to choose a path, or alias or whatever he wants as the string used to identify the wallet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See discussion in #10650 "The walletID is currently defined by the filename (-wallet=), ideally, we later add a wallet RPC call to set the walletID (should be written to the wallet database)." It would still be possible to do this if you are interested, but it's tangential to this PR.
There is more discussion starting at "The walletID should in future not be a representation of the filename. User should be able to manually choose the walletID. Ideally, we write the walletID into the wallet database. There we could only allow alphanumeric chars." #10650 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
listwallets will expose the full path right?
No, or at least not by default. Listwallets just shows the wallet name. If the server is configured to use full paths for wallet name these will be full paths, but the server has to be specifically configured this way in order for this to happen.
I'm still little puzzled about what you think the underlying problem is with allowing server admins to use full paths as wallet names. But if you really think it is a problem it would literally be a one-line code change to use a different identifier. E.g. the name
argument to CreateWalletFromFile()
could be changed from walletFile
to fs::path(walletFile).filename()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suppose you have configured the server with an absolute wallet path. Then you have a couple of clients (in different hosts). You had to configure each client with the absolute path of the wallet. These clients can be others than bitcoin-cli. Now the server operator decides it has to move the wallet to a new location. The clients can handle a server restart, but now the wallet doesn't exists. And the operator simply doesn't know of these clients because they are managed by other operators etc..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point is to not even allow such cases. An wallet id (filename or even relative path) is enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suppose you have configured the server with an absolute wallet path. Then you have a couple of clients (in different hosts). You had to configure each client with the absolute path of the wallet. These clients can be others than bitcoin-cli. Now the server operator decides it has to move the wallet to a new location.
I don't see how this is much different than the server renaming the wallet. If a server admin wants to create a stable name for a wallet, it's perfectly possible to do that by creating a symlink or just manipulating the -datadir or -walletdir (#11687) settings to be able store the wallets in different locations without changing the name. I believe this PR actually gives server admins more flexibility in this regard rather than less, because previously wallet symlinks were disallowed.
But to take the example at face value, you have a server admin who accidentally exposed a non-stable wallet name, instead of a stable wallet name over the rpc interface to clients on the internet outside of his control. What solutions would you like to see for this problem? Would it be good enough to use fs::path(walletFile).filename()
instead of walletFile
as the wallet name passed to CreateWalletFromFile
like I suggested earlier? Do you want wallet names to be configurable on the command line? Do you want them to be configurable by RPC? Do you want wallet names to be stored inside wallet databases like Jonas suggested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accidentally exposed a non-stable wallet
My point is to prevent this. IMO symlinks are a weak solution to remedy that case. Renaming is indeed a breaking change from the client point of view.
I'm not a server admin expert (not a bit either) but, correct me if I'm wrong, usually absolute paths aren't exposed outside.
Note that this was posted in response to an earlier version of this pr that did not include the third commit. With the third commit, the default for creating new wallets is as directories rather than a files (with each wallet directory containing its own wallet.dat file, db.log and database/log files). See the comments and release notes in the third commit for details.
The PR doesn't do this (and never did). The first commit of the PR is a refactoring specifically to add support for opening multiple BDB environments.
Again, the very first commit of this PR adds support for multiple BDB environments, and if you combine this PR with #11466 the result is exactly what you described in your linked comment. |
Indeed, concept ACK on the new approach, will review soon |
c1e5d40 Make debugging test crash easier (MeshCollider) 8263f6a Create walletdir if datadir doesn't exist and fix tests (MeshCollider) 9587a9c Default walletdir is wallets/ if it exists (MeshCollider) d987889 Add release notes for -walletdir and wallets/ dir (MeshCollider) 80c5cbc Add test for -walletdir (MeshCollider) 0530ba0 Add -walletdir parameter to specify custom wallet dir (MeshCollider) Pull request description: Closes #11348 Adds a `-walletdir` parameter which specifies a directory to use for wallets, allowing them to be stored separately from the 'main' data directory. Creates a new `wallets/` directory in datadir if this is the first time running, and defaults to using it if it exists. Includes tests and release notes. Things which might need to be considered more: - there is no 'lock' on the wallets directory, which might be needed? - because this uses a new wallets/ directory by default, downgrading to an earlier version won't see the wallets in that directory (not a big deal though, users can just copy them up to the main dir) - jnewbery suggested putting each wallet in its own directory, which is a good idea, but out of scope for this PR IMO. EDIT: this is being done in #11687 - doc/files.md needs updating (will do soon) I also considered including a cleanup by removing caching of data directory paths and instead just initialise them once on startup (c.f. #3073), but decided it wasn't super relevant here will just complicate review. Tree-SHA512: c8ac04bfe9a810c32055f2c8b8fa0d535e56125ceb8d96f12447dd3538bf3e5ee992b60b1cd2173bf5f3fa023a9feab12c9963593bf27ed419df929bb413398d
IMO we should still forbid paths for file-based wallets, and only allow it for directory-based ones. Suggest renaming |
Concept ACK |
Sorry! I didn't read the code but was apparently confused by all the screaming about dangerousness. If you added support for multiple database environments you get my concept ACK.
I don't think we should be changing the default naming of wallet databases. People have been backing up separate wallet.dats forever, and generally it works because bitcoin core leaves the wallet in consolidated state. So I'd prefer documenting that behavior instead. Even a backup with the last update stripped is better than no backup. From that it's usually possible to recover due to the mempool or HD seed. But if they can't find 'wallet.dat', they might decide not to to a backup at all. Turning the abstraction in the user's mind of wallets into directories is going to be an upstream fight as well. I'm not against it, but I think it will result in a lot of confusion. |
Good idea, added check for this. |
src/wallet/db.h
Outdated
@@ -85,7 +86,8 @@ class CDBEnv | |||
} | |||
}; | |||
|
|||
extern CDBEnv bitdb; | |||
/** Get CDBEnv and database filename given a wallet path. */ | |||
void GetWalletEnv(const fs::path& wallet_path, CDBEnv*& env, std::string& database_filename); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, can we return the CDBEnv* instead of return-by-setting-ref-to-ptr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh. Good idea! Done in 75ed8c5
src/wallet/db.h
Outdated
{ | ||
GetWalletEnv(wallet_path, env, strFile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why create the env before checking for mock? Why not add a fMock parameter to GetWalletEnv or so instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see how that would be an improvement. The env
object needs to exist whether it is memory or file-backed. And GetWalletEnv
is just intended to create and retrieve CDBEnv objects, not do that and then invoke unrelated CDBEnv methods that only one of its callers needs depending on a bool parameter...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It just seems very weird given mock seems like it shouldn't have a/care what the wallet_path is. In the future I kinda feel like it should become more RAII (ie Open in the GetWalletEnv call) but not in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It just seems very weird given mock seems like it shouldn't have a/care what the wallet_path is. In the future I kinda feel like it should become more RAII (ie Open in the GetWalletEnv call) but not in this PR.
RAII is not the model this uses because when there more than one wallet file open in the same directory they share the same environment. You could only resolve this by requiring all databases in the environment to be opened and closed at the same time (which would complicate dynamic loading/unloading) or by not allowing multiple databases in the same directory to be opened at the same time, or some combination these things, or just not using RAII.
On using fake "" path for mock databases, this is a little weird. Could be made less exposed by adding a separate constructor for mocks like proposed in the other comment: #11687 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but instead of env = GetWalletEnv(wallet_path, strFile); env->Open(); or env->Close(); env->MakeMock();, GetWalletEnv could return a reference which is already open or otherwise has some level of understanding of closing when all refs to that env are lost. Even if not that, would prefer to not use the fake "" path as you suggest.
src/wallet/db.h
Outdated
@@ -35,17 +35,18 @@ class CDBEnv | |||
void EnvShutdown(); | |||
|
|||
public: | |||
mutable CCriticalSection cs_db; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why make the lock global instead of per-env? It seems like there should be nothing preventing you from keeping that and adding a second lock that only exists to protect g_dbenvs, though likely not a big deal in terms of concurrency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One lock seems a simpler than multiple multiple locks unless there is a performance rationale for more fine grained locking. One thing which I would not look forward to is dealing with even more lock-order bugs in the wallet than the ones that already frequently arise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It just seems weird given it'd be even less code change to not split the lock out, and as long as the global lock is only held in the creation of new envs, no lockorder should be worried about (cause lockorder is always trivially correct if locks are held only for the duration of a function which uses them, without making callbacks).
src/wallet/db.cpp
Outdated
} | ||
|
||
void CDBEnv::Close() | ||
{ | ||
for (auto& db : mapDb) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While you're at it, can you just merge EnvShutdown and Close? EnvShutdown is now only called from Close().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 75ed8c5
src/wallet/db.cpp
Outdated
@@ -435,7 +467,9 @@ CDB::CDB(CWalletDBWrapper& dbw, const char* pszMode, bool fFlushOnCloseIn) : pdb | |||
if (ret != 0) { | |||
throw std::runtime_error(strprintf("CDB: Error %d, can't open database %s", ret, strFilename)); | |||
} | |||
CheckUniqueFileid(*env, strFilename, *pdb_temp); | |||
for (auto& env : g_dbenvs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this required? Somehow I had assumed they only conficted if they were both in the same env, but I dunno where I got that from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comment in 75ed8c5. It's done to prevent opening two paths that refer to the same file at the same time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you forget to push? Noting this is the only way we handle hardlinks is useful, indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you forget to push?
Oops, yeah. It was on the pr/wfile.9 tag I linked to below. Should be updated now. Also I'll work on the "just leave this part of the diff" change you requested above.
src/wallet/wallet.cpp
Outdated
// needed to restore wallet transaction meta data after -zapwallettxes | ||
std::vector<CWalletTx> vWtx; | ||
|
||
if (gArgs.GetBoolArg("-zapwallettxes", false)) { | ||
uiInterface.InitMessage(_("Zapping all transactions from wallet...")); | ||
|
||
std::unique_ptr<CWalletDBWrapper> dbw(new CWalletDBWrapper(&bitdb, walletFile)); | ||
std::unique_ptr<CWallet> tempWallet = MakeUnique<CWallet>(std::move(dbw)); | ||
std::unique_ptr<CWallet> tempWallet = MakeUnique<CWallet>(name, path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, I have to admit I really prefer the old version from the new one. Creating a DB and then giving it to the wallet makes more sense conceptually to me than telling the wallet where to find the DB and then having the CWallet constructor create the DB (especially as the slow-moving goal is to abstract the DB out of the wallet and make it an interface).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, I guess you are imagining CWalletDBWrapper
changing into some abstract interface that the wallet can call to load and store data. If that's the goal I can make the CWallet(name, dbw)
constructor public instead of private and get rid of the other constructors. For callers, this means the current:
MakeUnique<CWallet>(name, path)
MakeUnique<CWallet>(CWallet::Mock())
MakeUnique<CWallet>(CWallet::Dummy()))
would become something like:
MakeUnique<CWallet>(name, MakeUnique<CWalletDBWrapper>(path))
MakeUnique<CWallet>("mock", MakeUnique<CWalletDBWrapper>(CWalletDBWrapper::Mock()))
MakeUnique<CWallet>("dummy", MakeUnique<CWalletDBWrapper>(CWalletDBWrapper::Dummy()))
I would probably want to hold off on a change like this until CWalletDBWrapper
did become a real abstraction. But let me know if you want this change or something along these lines. Obviously this is more verbose than code in the current pr, but it's not much more verbose than code currently in master.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I really like that, tbh. If nothing else is much more clearly separates wallet.cpp from walletdb, etc, keeping wallet.cpp/CWallet only filled with wallet logic, and not database-backend stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If nothing else is much more clearly separates wallet.cpp from walletdb, etc, keeping wallet.cpp/CWallet only filled with wallet logic
It really affects callers of wallet, not wallet itself. CWallet is barely affected and wallet.cpp doesn't change at all. Anyway, let me know if you would prefer to see this here or in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean given you're changing it here, I see no reason to change it to go in the opposite direction of where I think it should go - even if CWalletDBWrapper right now is just a BDB implementation, it still has a clean-ish interface, and having the CWallet cosntructor create one from a path instead of take one just seems like the wrong direction. Better to just leave this part of the diff out of the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 39fa1e9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added 2 commits 2cf4dc8 -> ec01271 (pr/wfile.7 -> pr/wfile.8, compare)
Squashed ec01271 -> c25ed1f (pr/wfile.8 -> pr/wfile.9)
src/wallet/db.cpp
Outdated
} | ||
|
||
void CDBEnv::Close() | ||
{ | ||
for (auto& db : mapDb) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 75ed8c5
src/wallet/db.cpp
Outdated
@@ -435,7 +467,9 @@ CDB::CDB(CWalletDBWrapper& dbw, const char* pszMode, bool fFlushOnCloseIn) : pdb | |||
if (ret != 0) { | |||
throw std::runtime_error(strprintf("CDB: Error %d, can't open database %s", ret, strFilename)); | |||
} | |||
CheckUniqueFileid(*env, strFilename, *pdb_temp); | |||
for (auto& env : g_dbenvs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comment in 75ed8c5. It's done to prevent opening two paths that refer to the same file at the same time.
src/wallet/db.h
Outdated
@@ -35,17 +35,18 @@ class CDBEnv | |||
void EnvShutdown(); | |||
|
|||
public: | |||
mutable CCriticalSection cs_db; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One lock seems a simpler than multiple multiple locks unless there is a performance rationale for more fine grained locking. One thing which I would not look forward to is dealing with even more lock-order bugs in the wallet than the ones that already frequently arise.
src/wallet/db.h
Outdated
@@ -85,7 +86,8 @@ class CDBEnv | |||
} | |||
}; | |||
|
|||
extern CDBEnv bitdb; | |||
/** Get CDBEnv and database filename given a wallet path. */ | |||
void GetWalletEnv(const fs::path& wallet_path, CDBEnv*& env, std::string& database_filename); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh. Good idea! Done in 75ed8c5
src/wallet/db.h
Outdated
{ | ||
GetWalletEnv(wallet_path, env, strFile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see how that would be an improvement. The env
object needs to exist whether it is memory or file-backed. And GetWalletEnv
is just intended to create and retrieve CDBEnv objects, not do that and then invoke unrelated CDBEnv methods that only one of its callers needs depending on a bool parameter...
src/wallet/wallet.cpp
Outdated
// needed to restore wallet transaction meta data after -zapwallettxes | ||
std::vector<CWalletTx> vWtx; | ||
|
||
if (gArgs.GetBoolArg("-zapwallettxes", false)) { | ||
uiInterface.InitMessage(_("Zapping all transactions from wallet...")); | ||
|
||
std::unique_ptr<CWalletDBWrapper> dbw(new CWalletDBWrapper(&bitdb, walletFile)); | ||
std::unique_ptr<CWallet> tempWallet = MakeUnique<CWallet>(std::move(dbw)); | ||
std::unique_ptr<CWallet> tempWallet = MakeUnique<CWallet>(name, path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, I guess you are imagining CWalletDBWrapper
changing into some abstract interface that the wallet can call to load and store data. If that's the goal I can make the CWallet(name, dbw)
constructor public instead of private and get rid of the other constructors. For callers, this means the current:
MakeUnique<CWallet>(name, path)
MakeUnique<CWallet>(CWallet::Mock())
MakeUnique<CWallet>(CWallet::Dummy()))
would become something like:
MakeUnique<CWallet>(name, MakeUnique<CWalletDBWrapper>(path))
MakeUnique<CWallet>("mock", MakeUnique<CWalletDBWrapper>(CWalletDBWrapper::Mock()))
MakeUnique<CWallet>("dummy", MakeUnique<CWalletDBWrapper>(CWalletDBWrapper::Dummy()))
I would probably want to hold off on a change like this until CWalletDBWrapper
did become a real abstraction. But let me know if you want this change or something along these lines. Obviously this is more verbose than code in the current pr, but it's not much more verbose than code currently in master.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added 2 commits c25ed1f -> b9440ab (pr/wfile.9 -> pr/wfile.10, compare)
Squashed b9440ab -> 9993492 (pr/wfile.10 -> pr/wfile.11)
src/wallet/wallet.cpp
Outdated
// needed to restore wallet transaction meta data after -zapwallettxes | ||
std::vector<CWalletTx> vWtx; | ||
|
||
if (gArgs.GetBoolArg("-zapwallettxes", false)) { | ||
uiInterface.InitMessage(_("Zapping all transactions from wallet...")); | ||
|
||
std::unique_ptr<CWalletDBWrapper> dbw(new CWalletDBWrapper(&bitdb, walletFile)); | ||
std::unique_ptr<CWallet> tempWallet = MakeUnique<CWallet>(std::move(dbw)); | ||
std::unique_ptr<CWallet> tempWallet = MakeUnique<CWallet>(name, path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 39fa1e9
be8ab7d Create new wallet databases as directories rather than files (Russell Yanofsky) 26c06f2 Allow wallet files not in -walletdir directory (Russell Yanofsky) d8a99f6 Allow wallet files in multiple directories (Russell Yanofsky) Pull request description: This change consists of three commits: * The first commit is a pure refactoring that removes the restriction that two wallets can only be opened at the same time if they are contained in the same directory. * The second commit removes the restriction that `-wallet` filenames can only refer to files in the `-walletdir` directory. * The third commit makes second commit a little safer by changing bitcoin to create wallet databases as directories rather than files, so they can be safely backed up. All three commits should be straightforward: * The first commit adds around 20 lines of new code and then updates a bunch of function signatures (generally updating them to take plain fs::path parameters, instead of combinations of strings, fs::paths, and objects like CDBEnv and CWalletDBWrapper). * The second commit removes two `-wallet` filename checks and adds some test cases to the multiwallet unit test. * The third commit just changes the mapping from specified wallet paths to bdb environment & data paths. --- **Note:** For anybody looking at this PR for the first time, I think you can skip the comments before _20 Nov_ and start reading at bitcoin#11687 (comment). Comments before _20 Nov_ were about an earlier version of the PR that didn't include the third commit, and then confusion from not seeing the first commit. Tree-SHA512: 00bbb120fe0df847cf57014f75f1f7f1f58b0b62fa0b3adab4560163ebdfe06ccdfff33b4231693f03c5dc23601cb41954a07bcea9a4919c8d42f7d62bcf6024
be8ab7d Create new wallet databases as directories rather than files (Russell Yanofsky) 26c06f2 Allow wallet files not in -walletdir directory (Russell Yanofsky) d8a99f6 Allow wallet files in multiple directories (Russell Yanofsky) Pull request description: This change consists of three commits: * The first commit is a pure refactoring that removes the restriction that two wallets can only be opened at the same time if they are contained in the same directory. * The second commit removes the restriction that `-wallet` filenames can only refer to files in the `-walletdir` directory. * The third commit makes second commit a little safer by changing bitcoin to create wallet databases as directories rather than files, so they can be safely backed up. All three commits should be straightforward: * The first commit adds around 20 lines of new code and then updates a bunch of function signatures (generally updating them to take plain fs::path parameters, instead of combinations of strings, fs::paths, and objects like CDBEnv and CWalletDBWrapper). * The second commit removes two `-wallet` filename checks and adds some test cases to the multiwallet unit test. * The third commit just changes the mapping from specified wallet paths to bdb environment & data paths. --- **Note:** For anybody looking at this PR for the first time, I think you can skip the comments before _20 Nov_ and start reading at bitcoin#11687 (comment). Comments before _20 Nov_ were about an earlier version of the PR that didn't include the third commit, and then confusion from not seeing the first commit. Tree-SHA512: 00bbb120fe0df847cf57014f75f1f7f1f58b0b62fa0b3adab4560163ebdfe06ccdfff33b4231693f03c5dc23601cb41954a07bcea9a4919c8d42f7d62bcf6024
be8ab7d Create new wallet databases as directories rather than files (Russell Yanofsky) 26c06f2 Allow wallet files not in -walletdir directory (Russell Yanofsky) d8a99f6 Allow wallet files in multiple directories (Russell Yanofsky) Pull request description: This change consists of three commits: * The first commit is a pure refactoring that removes the restriction that two wallets can only be opened at the same time if they are contained in the same directory. * The second commit removes the restriction that `-wallet` filenames can only refer to files in the `-walletdir` directory. * The third commit makes second commit a little safer by changing bitcoin to create wallet databases as directories rather than files, so they can be safely backed up. All three commits should be straightforward: * The first commit adds around 20 lines of new code and then updates a bunch of function signatures (generally updating them to take plain fs::path parameters, instead of combinations of strings, fs::paths, and objects like CDBEnv and CWalletDBWrapper). * The second commit removes two `-wallet` filename checks and adds some test cases to the multiwallet unit test. * The third commit just changes the mapping from specified wallet paths to bdb environment & data paths. --- **Note:** For anybody looking at this PR for the first time, I think you can skip the comments before _20 Nov_ and start reading at bitcoin#11687 (comment). Comments before _20 Nov_ were about an earlier version of the PR that didn't include the third commit, and then confusion from not seeing the first commit. Tree-SHA512: 00bbb120fe0df847cf57014f75f1f7f1f58b0b62fa0b3adab4560163ebdfe06ccdfff33b4231693f03c5dc23601cb41954a07bcea9a4919c8d42f7d62bcf6024
be8ab7d Create new wallet databases as directories rather than files (Russell Yanofsky) 26c06f2 Allow wallet files not in -walletdir directory (Russell Yanofsky) d8a99f6 Allow wallet files in multiple directories (Russell Yanofsky) Pull request description: This change consists of three commits: * The first commit is a pure refactoring that removes the restriction that two wallets can only be opened at the same time if they are contained in the same directory. * The second commit removes the restriction that `-wallet` filenames can only refer to files in the `-walletdir` directory. * The third commit makes second commit a little safer by changing bitcoin to create wallet databases as directories rather than files, so they can be safely backed up. All three commits should be straightforward: * The first commit adds around 20 lines of new code and then updates a bunch of function signatures (generally updating them to take plain fs::path parameters, instead of combinations of strings, fs::paths, and objects like CDBEnv and CWalletDBWrapper). * The second commit removes two `-wallet` filename checks and adds some test cases to the multiwallet unit test. * The third commit just changes the mapping from specified wallet paths to bdb environment & data paths. --- **Note:** For anybody looking at this PR for the first time, I think you can skip the comments before _20 Nov_ and start reading at bitcoin#11687 (comment). Comments before _20 Nov_ were about an earlier version of the PR that didn't include the third commit, and then confusion from not seeing the first commit. Tree-SHA512: 00bbb120fe0df847cf57014f75f1f7f1f58b0b62fa0b3adab4560163ebdfe06ccdfff33b4231693f03c5dc23601cb41954a07bcea9a4919c8d42f7d62bcf6024
be8ab7d Create new wallet databases as directories rather than files (Russell Yanofsky) 26c06f2 Allow wallet files not in -walletdir directory (Russell Yanofsky) d8a99f6 Allow wallet files in multiple directories (Russell Yanofsky) Pull request description: This change consists of three commits: * The first commit is a pure refactoring that removes the restriction that two wallets can only be opened at the same time if they are contained in the same directory. * The second commit removes the restriction that `-wallet` filenames can only refer to files in the `-walletdir` directory. * The third commit makes second commit a little safer by changing bitcoin to create wallet databases as directories rather than files, so they can be safely backed up. All three commits should be straightforward: * The first commit adds around 20 lines of new code and then updates a bunch of function signatures (generally updating them to take plain fs::path parameters, instead of combinations of strings, fs::paths, and objects like CDBEnv and CWalletDBWrapper). * The second commit removes two `-wallet` filename checks and adds some test cases to the multiwallet unit test. * The third commit just changes the mapping from specified wallet paths to bdb environment & data paths. --- **Note:** For anybody looking at this PR for the first time, I think you can skip the comments before _20 Nov_ and start reading at bitcoin#11687 (comment). Comments before _20 Nov_ were about an earlier version of the PR that didn't include the third commit, and then confusion from not seeing the first commit. Tree-SHA512: 00bbb120fe0df847cf57014f75f1f7f1f58b0b62fa0b3adab4560163ebdfe06ccdfff33b4231693f03c5dc23601cb41954a07bcea9a4919c8d42f7d62bcf6024
be8ab7d Create new wallet databases as directories rather than files (Russell Yanofsky) 26c06f2 Allow wallet files not in -walletdir directory (Russell Yanofsky) d8a99f6 Allow wallet files in multiple directories (Russell Yanofsky) Pull request description: This change consists of three commits: * The first commit is a pure refactoring that removes the restriction that two wallets can only be opened at the same time if they are contained in the same directory. * The second commit removes the restriction that `-wallet` filenames can only refer to files in the `-walletdir` directory. * The third commit makes second commit a little safer by changing bitcoin to create wallet databases as directories rather than files, so they can be safely backed up. All three commits should be straightforward: * The first commit adds around 20 lines of new code and then updates a bunch of function signatures (generally updating them to take plain fs::path parameters, instead of combinations of strings, fs::paths, and objects like CDBEnv and CWalletDBWrapper). * The second commit removes two `-wallet` filename checks and adds some test cases to the multiwallet unit test. * The third commit just changes the mapping from specified wallet paths to bdb environment & data paths. --- **Note:** For anybody looking at this PR for the first time, I think you can skip the comments before _20 Nov_ and start reading at bitcoin#11687 (comment). Comments before _20 Nov_ were about an earlier version of the PR that didn't include the third commit, and then confusion from not seeing the first commit. Tree-SHA512: 00bbb120fe0df847cf57014f75f1f7f1f58b0b62fa0b3adab4560163ebdfe06ccdfff33b4231693f03c5dc23601cb41954a07bcea9a4919c8d42f7d62bcf6024
be8ab7d Create new wallet databases as directories rather than files (Russell Yanofsky) 26c06f2 Allow wallet files not in -walletdir directory (Russell Yanofsky) d8a99f6 Allow wallet files in multiple directories (Russell Yanofsky) Pull request description: This change consists of three commits: * The first commit is a pure refactoring that removes the restriction that two wallets can only be opened at the same time if they are contained in the same directory. * The second commit removes the restriction that `-wallet` filenames can only refer to files in the `-walletdir` directory. * The third commit makes second commit a little safer by changing bitcoin to create wallet databases as directories rather than files, so they can be safely backed up. All three commits should be straightforward: * The first commit adds around 20 lines of new code and then updates a bunch of function signatures (generally updating them to take plain fs::path parameters, instead of combinations of strings, fs::paths, and objects like CDBEnv and CWalletDBWrapper). * The second commit removes two `-wallet` filename checks and adds some test cases to the multiwallet unit test. * The third commit just changes the mapping from specified wallet paths to bdb environment & data paths. --- **Note:** For anybody looking at this PR for the first time, I think you can skip the comments before _20 Nov_ and start reading at bitcoin#11687 (comment). Comments before _20 Nov_ were about an earlier version of the PR that didn't include the third commit, and then confusion from not seeing the first commit. Tree-SHA512: 00bbb120fe0df847cf57014f75f1f7f1f58b0b62fa0b3adab4560163ebdfe06ccdfff33b4231693f03c5dc23601cb41954a07bcea9a4919c8d42f7d62bcf6024
be8ab7d Create new wallet databases as directories rather than files (Russell Yanofsky) 26c06f2 Allow wallet files not in -walletdir directory (Russell Yanofsky) d8a99f6 Allow wallet files in multiple directories (Russell Yanofsky) Pull request description: This change consists of three commits: * The first commit is a pure refactoring that removes the restriction that two wallets can only be opened at the same time if they are contained in the same directory. * The second commit removes the restriction that `-wallet` filenames can only refer to files in the `-walletdir` directory. * The third commit makes second commit a little safer by changing bitcoin to create wallet databases as directories rather than files, so they can be safely backed up. All three commits should be straightforward: * The first commit adds around 20 lines of new code and then updates a bunch of function signatures (generally updating them to take plain fs::path parameters, instead of combinations of strings, fs::paths, and objects like CDBEnv and CWalletDBWrapper). * The second commit removes two `-wallet` filename checks and adds some test cases to the multiwallet unit test. * The third commit just changes the mapping from specified wallet paths to bdb environment & data paths. --- **Note:** For anybody looking at this PR for the first time, I think you can skip the comments before _20 Nov_ and start reading at bitcoin#11687 (comment). Comments before _20 Nov_ were about an earlier version of the PR that didn't include the third commit, and then confusion from not seeing the first commit. Tree-SHA512: 00bbb120fe0df847cf57014f75f1f7f1f58b0b62fa0b3adab4560163ebdfe06ccdfff33b4231693f03c5dc23601cb41954a07bcea9a4919c8d42f7d62bcf6024
be8ab7d Create new wallet databases as directories rather than files (Russell Yanofsky) 26c06f2 Allow wallet files not in -walletdir directory (Russell Yanofsky) d8a99f6 Allow wallet files in multiple directories (Russell Yanofsky) Pull request description: This change consists of three commits: * The first commit is a pure refactoring that removes the restriction that two wallets can only be opened at the same time if they are contained in the same directory. * The second commit removes the restriction that `-wallet` filenames can only refer to files in the `-walletdir` directory. * The third commit makes second commit a little safer by changing bitcoin to create wallet databases as directories rather than files, so they can be safely backed up. All three commits should be straightforward: * The first commit adds around 20 lines of new code and then updates a bunch of function signatures (generally updating them to take plain fs::path parameters, instead of combinations of strings, fs::paths, and objects like CDBEnv and CWalletDBWrapper). * The second commit removes two `-wallet` filename checks and adds some test cases to the multiwallet unit test. * The third commit just changes the mapping from specified wallet paths to bdb environment & data paths. --- **Note:** For anybody looking at this PR for the first time, I think you can skip the comments before _20 Nov_ and start reading at bitcoin#11687 (comment). Comments before _20 Nov_ were about an earlier version of the PR that didn't include the third commit, and then confusion from not seeing the first commit. Tree-SHA512: 00bbb120fe0df847cf57014f75f1f7f1f58b0b62fa0b3adab4560163ebdfe06ccdfff33b4231693f03c5dc23601cb41954a07bcea9a4919c8d42f7d62bcf6024
be8ab7d Create new wallet databases as directories rather than files (Russell Yanofsky) 26c06f2 Allow wallet files not in -walletdir directory (Russell Yanofsky) d8a99f6 Allow wallet files in multiple directories (Russell Yanofsky) Pull request description: This change consists of three commits: * The first commit is a pure refactoring that removes the restriction that two wallets can only be opened at the same time if they are contained in the same directory. * The second commit removes the restriction that `-wallet` filenames can only refer to files in the `-walletdir` directory. * The third commit makes second commit a little safer by changing bitcoin to create wallet databases as directories rather than files, so they can be safely backed up. All three commits should be straightforward: * The first commit adds around 20 lines of new code and then updates a bunch of function signatures (generally updating them to take plain fs::path parameters, instead of combinations of strings, fs::paths, and objects like CDBEnv and CWalletDBWrapper). * The second commit removes two `-wallet` filename checks and adds some test cases to the multiwallet unit test. * The third commit just changes the mapping from specified wallet paths to bdb environment & data paths. --- **Note:** For anybody looking at this PR for the first time, I think you can skip the comments before _20 Nov_ and start reading at bitcoin#11687 (comment). Comments before _20 Nov_ were about an earlier version of the PR that didn't include the third commit, and then confusion from not seeing the first commit. Tree-SHA512: 00bbb120fe0df847cf57014f75f1f7f1f58b0b62fa0b3adab4560163ebdfe06ccdfff33b4231693f03c5dc23601cb41954a07bcea9a4919c8d42f7d62bcf6024
be8ab7d Create new wallet databases as directories rather than files (Russell Yanofsky) 26c06f2 Allow wallet files not in -walletdir directory (Russell Yanofsky) d8a99f6 Allow wallet files in multiple directories (Russell Yanofsky) Pull request description: This change consists of three commits: * The first commit is a pure refactoring that removes the restriction that two wallets can only be opened at the same time if they are contained in the same directory. * The second commit removes the restriction that `-wallet` filenames can only refer to files in the `-walletdir` directory. * The third commit makes second commit a little safer by changing bitcoin to create wallet databases as directories rather than files, so they can be safely backed up. All three commits should be straightforward: * The first commit adds around 20 lines of new code and then updates a bunch of function signatures (generally updating them to take plain fs::path parameters, instead of combinations of strings, fs::paths, and objects like CDBEnv and CWalletDBWrapper). * The second commit removes two `-wallet` filename checks and adds some test cases to the multiwallet unit test. * The third commit just changes the mapping from specified wallet paths to bdb environment & data paths. --- **Note:** For anybody looking at this PR for the first time, I think you can skip the comments before _20 Nov_ and start reading at bitcoin#11687 (comment). Comments before _20 Nov_ were about an earlier version of the PR that didn't include the third commit, and then confusion from not seeing the first commit. Tree-SHA512: 00bbb120fe0df847cf57014f75f1f7f1f58b0b62fa0b3adab4560163ebdfe06ccdfff33b4231693f03c5dc23601cb41954a07bcea9a4919c8d42f7d62bcf6024
be8ab7d Create new wallet databases as directories rather than files (Russell Yanofsky) 26c06f2 Allow wallet files not in -walletdir directory (Russell Yanofsky) d8a99f6 Allow wallet files in multiple directories (Russell Yanofsky) Pull request description: This change consists of three commits: * The first commit is a pure refactoring that removes the restriction that two wallets can only be opened at the same time if they are contained in the same directory. * The second commit removes the restriction that `-wallet` filenames can only refer to files in the `-walletdir` directory. * The third commit makes second commit a little safer by changing bitcoin to create wallet databases as directories rather than files, so they can be safely backed up. All three commits should be straightforward: * The first commit adds around 20 lines of new code and then updates a bunch of function signatures (generally updating them to take plain fs::path parameters, instead of combinations of strings, fs::paths, and objects like CDBEnv and CWalletDBWrapper). * The second commit removes two `-wallet` filename checks and adds some test cases to the multiwallet unit test. * The third commit just changes the mapping from specified wallet paths to bdb environment & data paths. --- **Note:** For anybody looking at this PR for the first time, I think you can skip the comments before _20 Nov_ and start reading at bitcoin#11687 (comment). Comments before _20 Nov_ were about an earlier version of the PR that didn't include the third commit, and then confusion from not seeing the first commit. Tree-SHA512: 00bbb120fe0df847cf57014f75f1f7f1f58b0b62fa0b3adab4560163ebdfe06ccdfff33b4231693f03c5dc23601cb41954a07bcea9a4919c8d42f7d62bcf6024
be8ab7d Create new wallet databases as directories rather than files (Russell Yanofsky) 26c06f2 Allow wallet files not in -walletdir directory (Russell Yanofsky) d8a99f6 Allow wallet files in multiple directories (Russell Yanofsky) Pull request description: This change consists of three commits: * The first commit is a pure refactoring that removes the restriction that two wallets can only be opened at the same time if they are contained in the same directory. * The second commit removes the restriction that `-wallet` filenames can only refer to files in the `-walletdir` directory. * The third commit makes second commit a little safer by changing bitcoin to create wallet databases as directories rather than files, so they can be safely backed up. All three commits should be straightforward: * The first commit adds around 20 lines of new code and then updates a bunch of function signatures (generally updating them to take plain fs::path parameters, instead of combinations of strings, fs::paths, and objects like CDBEnv and CWalletDBWrapper). * The second commit removes two `-wallet` filename checks and adds some test cases to the multiwallet unit test. * The third commit just changes the mapping from specified wallet paths to bdb environment & data paths. --- **Note:** For anybody looking at this PR for the first time, I think you can skip the comments before _20 Nov_ and start reading at bitcoin#11687 (comment). Comments before _20 Nov_ were about an earlier version of the PR that didn't include the third commit, and then confusion from not seeing the first commit. Tree-SHA512: 00bbb120fe0df847cf57014f75f1f7f1f58b0b62fa0b3adab4560163ebdfe06ccdfff33b4231693f03c5dc23601cb41954a07bcea9a4919c8d42f7d62bcf6024
…d use it for new descriptor wallets c4a29d0 Update wallet_multiwallet.py for descriptor and sqlite wallets (Russell Yanofsky) 310b0fd Run dumpwallet for legacy wallets only in wallet_backup.py (Andrew Chow) 6c6639a Include sqlite3 in documentation (Andrew Chow) f023b7c wallet: Enforce sqlite serialized threading mode (Andrew Chow) 6173269 Set and check the sqlite user version (Andrew Chow) 9d3d2d2 Use network magic as sqlite wallet application ID (Andrew Chow) 9af5de3 Use SQLite for descriptor wallets (Andrew Chow) 9b78f3c walletutil: Wallets can also be sqlite (Andrew Chow) ac38a87 Determine wallet file type based on file magic (Andrew Chow) 6045f77 Implement SQLiteDatabase::MakeBatch (Andrew Chow) 727e6b2 Implement SQLiteDatabase::Verify (Andrew Chow) b4df8fd Implement SQLiteDatabase::Rewrite (Andrew Chow) 010e365 Implement SQLiteDatabase::TxnBegin, TxnCommit, and TxnAbort (Andrew Chow) ac5c161 Implement SQLiteDatabase::Backup (Andrew Chow) f6f9cd6 Implement SQLiteBatch::StartCursor, ReadAtCursor, and CloseCursor (Andrew Chow) bf90e03 Implement SQLiteBatch::ReadKey, WriteKey, EraseKey, and HasKey (Andrew Chow) 7aa4562 Add SetupSQLStatements (Andrew Chow) 6636a26 Implement SQLiteBatch::Close (Andrew Chow) 9382535 Implement SQLiteDatabase::Close (Andrew Chow) a0de833 Implement SQLiteDatabase::Open (Andrew Chow) 3bfa0fe Initialize and Shutdown sqlite3 globals (Andrew Chow) 5a488b3 Constructors, destructors, and relevant private fields for SQLiteDatabase/Batch (Andrew Chow) ca8b7e0 Implement SQLiteDatabaseVersion (Andrew Chow) 7577b6e Add SQLiteDatabase and SQLiteBatch dummy classes (Andrew Chow) e87df82 Add sqlite to travis and depends (Andrew Chow) 54729f3 Add libsqlite3 (Andrew Chow) Pull request description: This PR adds a new class `SQLiteDatabase` which is a subclass of `WalletDatabase`. This provides access to a SQLite database that is used to store the wallet records. To keep compatibility with BDB and to complexity of the change down, we don't make use of many SQLite's features. We use it strictly as a key-value store. We create a table `main` which has two columns, `key` and `value` both with the type `blob`. For new descriptor wallets, we will create a `SQLiteDatabase` instead of a `BerkeleyDatabase`. There is no requirement that all SQLite wallets are descriptor wallets, nor is there a requirement that all descriptor wallets be SQLite wallets. This allows for existing descriptor wallets to work as well as keeping open the option to migrate existing wallets to SQLite. We keep the name `wallet.dat` for SQLite wallets. We are able to determine which database type to use by searching for specific magic bytes in the `wallet.dat` file. SQLite begins it's files with a null terminated string `SQLite format 3`. BDB has `0x00053162` at byte 12 (note that the byte order of this integer depends on the system endianness). So when we see that there is a `wallet.dat` file that we want to open, we check for the magic bytes to determine which database system to use. I decided to keep the `wallet.dat` naming to keep things like backup script to continue to function as they won't need to be modified to look for a different file name. It also simplifies a couple of things in the implementation and the tests as `wallet.dat` is something that is specifically being looked for. If we don't want this behavior, then I do have another branch which creates `wallet.sqlite` files instead, but I find that this direction is easier. ACKs for top commit: Sjors: re-utACK c4a29d0 promag: Tested ACK c4a29d0. fjahr: reACK c4a29d0 S3RK: Re-review ACK c4a29d0 meshcollider: re-utACK c4a29d0 hebasto: re-ACK c4a29d0, only rebased since my [previous](#19077 (review)) review, verified with `git range-diff master d18892d c4a29d0`. ryanofsky: Code review ACK c4a29d0. I am honestly confused about reasons for locking into `wallet.dat` again when it's so easy now to use a clean format. I assume I'm just very dense, or there's some unstated reason, because the only thing that's been brought up are unrealistic compatibility scenarios (all require actively creating a wallet with non-default descriptor+sqlite option, then trying to using the descriptor+sqlite wallets with old software or scripts and ignoring the results) that we didn't pay attention to with previous PRs like #11687, which did not require any active interfaction. jonatack: ACK c4a29d0, debug builds and test runs after rebase to latest master @ c2c4dba, some manual testing creating, using, unloading and reloading a few different new sqlite descriptor wallets over several node restarts/shutdowns. Tree-SHA512: 19145732e5001484947352d3175a660b5102bc6e833f227a55bd41b9b2f4d92737bbed7cead64b75b509decf9e1408cd81c185ab1fb4b90561aee427c4f9751c
…base and use it for new descriptor wallets c4a29d0 Update wallet_multiwallet.py for descriptor and sqlite wallets (Russell Yanofsky) 310b0fd Run dumpwallet for legacy wallets only in wallet_backup.py (Andrew Chow) 6c6639a Include sqlite3 in documentation (Andrew Chow) f023b7c wallet: Enforce sqlite serialized threading mode (Andrew Chow) 6173269 Set and check the sqlite user version (Andrew Chow) 9d3d2d2 Use network magic as sqlite wallet application ID (Andrew Chow) 9af5de3 Use SQLite for descriptor wallets (Andrew Chow) 9b78f3c walletutil: Wallets can also be sqlite (Andrew Chow) ac38a87 Determine wallet file type based on file magic (Andrew Chow) 6045f77 Implement SQLiteDatabase::MakeBatch (Andrew Chow) 727e6b2 Implement SQLiteDatabase::Verify (Andrew Chow) b4df8fd Implement SQLiteDatabase::Rewrite (Andrew Chow) 010e365 Implement SQLiteDatabase::TxnBegin, TxnCommit, and TxnAbort (Andrew Chow) ac5c161 Implement SQLiteDatabase::Backup (Andrew Chow) f6f9cd6 Implement SQLiteBatch::StartCursor, ReadAtCursor, and CloseCursor (Andrew Chow) bf90e03 Implement SQLiteBatch::ReadKey, WriteKey, EraseKey, and HasKey (Andrew Chow) 7aa4562 Add SetupSQLStatements (Andrew Chow) 6636a26 Implement SQLiteBatch::Close (Andrew Chow) 9382535 Implement SQLiteDatabase::Close (Andrew Chow) a0de833 Implement SQLiteDatabase::Open (Andrew Chow) 3bfa0fe Initialize and Shutdown sqlite3 globals (Andrew Chow) 5a488b3 Constructors, destructors, and relevant private fields for SQLiteDatabase/Batch (Andrew Chow) ca8b7e0 Implement SQLiteDatabaseVersion (Andrew Chow) 7577b6e Add SQLiteDatabase and SQLiteBatch dummy classes (Andrew Chow) e87df82 Add sqlite to travis and depends (Andrew Chow) 54729f3 Add libsqlite3 (Andrew Chow) Pull request description: This PR adds a new class `SQLiteDatabase` which is a subclass of `WalletDatabase`. This provides access to a SQLite database that is used to store the wallet records. To keep compatibility with BDB and to complexity of the change down, we don't make use of many SQLite's features. We use it strictly as a key-value store. We create a table `main` which has two columns, `key` and `value` both with the type `blob`. For new descriptor wallets, we will create a `SQLiteDatabase` instead of a `BerkeleyDatabase`. There is no requirement that all SQLite wallets are descriptor wallets, nor is there a requirement that all descriptor wallets be SQLite wallets. This allows for existing descriptor wallets to work as well as keeping open the option to migrate existing wallets to SQLite. We keep the name `wallet.dat` for SQLite wallets. We are able to determine which database type to use by searching for specific magic bytes in the `wallet.dat` file. SQLite begins it's files with a null terminated string `SQLite format 3`. BDB has `0x00053162` at byte 12 (note that the byte order of this integer depends on the system endianness). So when we see that there is a `wallet.dat` file that we want to open, we check for the magic bytes to determine which database system to use. I decided to keep the `wallet.dat` naming to keep things like backup script to continue to function as they won't need to be modified to look for a different file name. It also simplifies a couple of things in the implementation and the tests as `wallet.dat` is something that is specifically being looked for. If we don't want this behavior, then I do have another branch which creates `wallet.sqlite` files instead, but I find that this direction is easier. ACKs for top commit: Sjors: re-utACK c4a29d0 promag: Tested ACK c4a29d0. fjahr: reACK c4a29d0 S3RK: Re-review ACK c4a29d0 meshcollider: re-utACK c4a29d0 hebasto: re-ACK c4a29d0, only rebased since my [previous](bitcoin#19077 (review)) review, verified with `git range-diff master d18892d c4a29d0`. ryanofsky: Code review ACK c4a29d0. I am honestly confused about reasons for locking into `wallet.dat` again when it's so easy now to use a clean format. I assume I'm just very dense, or there's some unstated reason, because the only thing that's been brought up are unrealistic compatibility scenarios (all require actively creating a wallet with non-default descriptor+sqlite option, then trying to using the descriptor+sqlite wallets with old software or scripts and ignoring the results) that we didn't pay attention to with previous PRs like bitcoin#11687, which did not require any active interfaction. jonatack: ACK c4a29d0, debug builds and test runs after rebase to latest master @ c2c4dba, some manual testing creating, using, unloading and reloading a few different new sqlite descriptor wallets over several node restarts/shutdowns. Tree-SHA512: 19145732e5001484947352d3175a660b5102bc6e833f227a55bd41b9b2f4d92737bbed7cead64b75b509decf9e1408cd81c185ab1fb4b90561aee427c4f9751c
13c3a65 Qt/Bugfix: fix handling default wallet with no name (João Barbosa) Pull request description: If one loads a wallet via RPC (`loadwallet w2`), then select w2, select back to the default wallet (which is an empty string), that default wallet cannot be access through the RPC console because the current code only points to the wallet endpoint if the wallet name is not empty. This is a quick fix that reenables accessing the default wallet in case an additional wallet has been loaded. Using "" for the default wallet may not be ideal in other cases and it may make more sense to change it at a deeper level (wallet.cpp). See discussion here which where the reasons for the current behaviour in master: bitcoin#11687 (comment) @jnewbery @promag @ryanofsky Tree-SHA512: 74b935886b4e4a6033a2f5e1f44bb69a252e31f4021e19a2054445a8e3e4db1d8ee256290850a84d8569d2d0e21412fce0170e7f0e881259156057587181ee05
13c3a65 Qt/Bugfix: fix handling default wallet with no name (João Barbosa) Pull request description: If one loads a wallet via RPC (`loadwallet w2`), then select w2, select back to the default wallet (which is an empty string), that default wallet cannot be access through the RPC console because the current code only points to the wallet endpoint if the wallet name is not empty. This is a quick fix that reenables accessing the default wallet in case an additional wallet has been loaded. Using "" for the default wallet may not be ideal in other cases and it may make more sense to change it at a deeper level (wallet.cpp). See discussion here which where the reasons for the current behaviour in master: bitcoin#11687 (comment) @jnewbery @promag @ryanofsky Tree-SHA512: 74b935886b4e4a6033a2f5e1f44bb69a252e31f4021e19a2054445a8e3e4db1d8ee256290850a84d8569d2d0e21412fce0170e7f0e881259156057587181ee05
…param c1e5d40 Make debugging test crash easier (MeshCollider) 8263f6a Create walletdir if datadir doesn't exist and fix tests (MeshCollider) 9587a9c Default walletdir is wallets/ if it exists (MeshCollider) d987889 Add release notes for -walletdir and wallets/ dir (MeshCollider) 80c5cbc Add test for -walletdir (MeshCollider) 0530ba0 Add -walletdir parameter to specify custom wallet dir (MeshCollider) Pull request description: Closes bitcoin#11348 Adds a `-walletdir` parameter which specifies a directory to use for wallets, allowing them to be stored separately from the 'main' data directory. Creates a new `wallets/` directory in datadir if this is the first time running, and defaults to using it if it exists. Includes tests and release notes. Things which might need to be considered more: - there is no 'lock' on the wallets directory, which might be needed? - because this uses a new wallets/ directory by default, downgrading to an earlier version won't see the wallets in that directory (not a big deal though, users can just copy them up to the main dir) - jnewbery suggested putting each wallet in its own directory, which is a good idea, but out of scope for this PR IMO. EDIT: this is being done in bitcoin#11687 - doc/files.md needs updating (will do soon) I also considered including a cleanup by removing caching of data directory paths and instead just initialise them once on startup (c.f. dashpay#3073), but decided it wasn't super relevant here will just complicate review. Tree-SHA512: c8ac04bfe9a810c32055f2c8b8fa0d535e56125ceb8d96f12447dd3538bf3e5ee992b60b1cd2173bf5f3fa023a9feab12c9963593bf27ed419df929bb413398d
a14dbff Allow multiwallet.py to be used with --usecli (Russell Yanofsky) f6ade9c [tests] allow tests to be run with --usecli (John Newbery) ff9a363 TestNodeCLI batch emulation (Russell Yanofsky) ca9085a Prevent TestNodeCLI.args mixups (Russell Yanofsky) fcfb952 Improve TestNodeCLI output parsing (Russell Yanofsky) Pull request description: Lack of test coverage was pointed out by @jnewbery in bitcoin#11687 (comment) Tree-SHA512: 5f10e31abad11a5edab0da4e2515e39547adb6ab9e55e50427ab2eb7ec9a43d6b896b579b15863e5edc9beee7d8bf1c84d9dabd247be0760a1b9ae39e1e8ee02
2f3bd47 Abstract directory locking into util.cpp (MeshCollider) 5260a4a Make .walletlock distinct from .lock (MeshCollider) 64226de Generalise walletdir lock error message for correctness (MeshCollider) c9ed4bd Add a test for wallet directory locking (MeshCollider) e60cb99 Add a lock to the wallet directory (MeshCollider) Pull request description: Fixes bitcoin#11888, needs a 0.16 milestone Also adds a test that the lock works. bitcoin#11687 will probably rework this to a per-wallet lock instead of just the walletdir, but this fixes the current issue Tree-SHA512: 98e52d67f820e3b8f919cf361ffbb7d928f1bd67603e0ed26c5076ea02d9b3a90c3535ddf7329f3b88171396fa28dd3c87adab3577a8a217bd1e4247bda99138
…xternal wallet files capabilities 056f4e8 [GUI] settings information widget setting the correct data directory. (furszy) f765611 bugfix: Remove dangling wallet env instance (João Barbosa) 524103f Implement and connect CWallet::GetPathToFile to GUI. (furszy) e7aa6bd [Refactor] First load all wallets, then back em (random-zebra) 91b112b [Refactor][Bug] Fix automatic backups, final code deduplication (random-zebra) 12a1e39 [BUG] Sanitize wallet name in GetUniqueWalletBackupName (random-zebra) 7aa251d wallet: Fix backupwallet for multiwallets (Daniel Kraft) 351d2c8 wallet_tests: mock wallet db. (furszy) 565abcd db: fix db path not removed from the open db environments map. (furszy) 4cae8dc test: Add unit test for LockDirectory Add a unit test for LockDirectory, introduced in btc#11281. (W. J. van der Laan) 16b4651 util: Fix multiple use of LockDirectory This commit fixes problems with calling LockDirectory multiple times on the same directory, or from multiple threads. It also fixes the build on OpenBSD. (W. J. van der Laan) 9ae619a Test: Use specific testing setups for wallet_zkeys_tests tests (furszy) d86cd4f wallet: Add missing check for backup to source wallet file. (furszy) d9e1c6b Abstract VerifyWalletPath and connect it to init and GUI. (furszy) 23458ca GUI: Implement and connect WalletModel::getWalletPath(). (furszy) c2d3a07 Create new wallet databases as directories rather than files (Russell Yanofsky) daa7fe5 Allow wallet files not in -walletdir directory (Russell Yanofsky) 9b2dae1 Allow wallet files in multiple directories (furszy) d36185a wallet: unify backup creation process. (furszy) 8b8725d wallet_tests: Use dummy wallet instance instead of wallet pointer. (furszy) 434ed75 Abstract LockDirectory into system.cpp (furszy) 6a0380a Make .walletlock distinct from .lock (MeshCollider) d8539bb Generalise walletdir lock error message for correctness (MeshCollider) ddcfd4a Enable test for wallet directory locking (furszy) a238a8d Add a lock to the wallet directory (MeshCollider) 1dc2219 Don't allow relative -walletdir paths (Russell Yanofsky) dcb43ea Create walletdir if datadir doesn't exist and correct tests (furszy) 03db5c8 Default walletdir is wallets/ if it exists (MeshCollider) 359b01d Add release notes for -walletdir and wallets/ dir (MeshCollider) 71a4701 Add -walletdir parameter to specify custom wallet dir (furszy) 5b31813 Use unique_ptr for dbenv (DbEnv) (practicalswift) a1bef4f Refactor: Modernize disallowed copy constructors/assignment (Dan Raviv) Pull request description: > Adding more flexibility in where the wallets directory can be located. Before, the wallet database files were stored at the top level of the PIVX data directory. Now the location of the wallets directory can be overridden by specifying a `-walletdir=<path>` option where `<path>` can be an absolute path to a directory or directory symlink. > >Another advantage of this change is that if two wallets are located in the same directory, they will now use their own BerkeleyDB environments instead using a shared environment. Using a shared environment makes it difficult to manage and back up wallets separately because transaction log files will contain a mix of data from all wallets in the environment. Coming from: * bitcoin#11351 -> Refactor: Modernize disallowed copy constructors/assignment. * bitcoin#11466 -> Specify custom wallet directory with -walletdir param. * bitcoin#11726 -> Cleanups + nit fixes for -walletdir work. * bitcoin#11904 -> Add lock to the wallet directory. * bitcoin#11687 -> External wallet files support. * bitcoin#12166 -> Doc better -walletdir description. * bitcoin#12220 -> Error if relative -walletdir is specified. This finishes the work started in #2337, enabling all the commented tests inside `wallet_multiwallet.py` and more. PR built on top of #2369. Note: Test this properly. ACKs for top commit: random-zebra: utACK 056f4e8 Fuzzbawls: ACK 056f4e8 Tree-SHA512: 98ae515dd664f959d35b63a0998bd93ca3bcea30ca67caccd2a694a440d10e18f831a54720ede0415d8f5e13af252bc6048a820491863d243df70ccc9d5fa7c6
c1e5d40 Make debugging test crash easier (MeshCollider) 8263f6a Create walletdir if datadir doesn't exist and fix tests (MeshCollider) 9587a9c Default walletdir is wallets/ if it exists (MeshCollider) d987889 Add release notes for -walletdir and wallets/ dir (MeshCollider) 80c5cbc Add test for -walletdir (MeshCollider) 0530ba0 Add -walletdir parameter to specify custom wallet dir (MeshCollider) Pull request description: Closes #11348 Adds a `-walletdir` parameter which specifies a directory to use for wallets, allowing them to be stored separately from the 'main' data directory. Creates a new `wallets/` directory in datadir if this is the first time running, and defaults to using it if it exists. Includes tests and release notes. Things which might need to be considered more: - there is no 'lock' on the wallets directory, which might be needed? - because this uses a new wallets/ directory by default, downgrading to an earlier version won't see the wallets in that directory (not a big deal though, users can just copy them up to the main dir) - jnewbery suggested putting each wallet in its own directory, which is a good idea, but out of scope for this PR IMO. EDIT: this is being done in bitcoin/bitcoin#11687 - doc/files.md needs updating (will do soon) I also considered including a cleanup by removing caching of data directory paths and instead just initialise them once on startup (c.f. #3073), but decided it wasn't super relevant here will just complicate review. Tree-SHA512: c8ac04bfe9a810c32055f2c8b8fa0d535e56125ceb8d96f12447dd3538bf3e5ee992b60b1cd2173bf5f3fa023a9feab12c9963593bf27ed419df929bb413398d
a14dbff39e Allow multiwallet.py to be used with --usecli (Russell Yanofsky) f6ade9ce1a [tests] allow tests to be run with --usecli (John Newbery) ff9a363ff7 TestNodeCLI batch emulation (Russell Yanofsky) ca9085afc5 Prevent TestNodeCLI.args mixups (Russell Yanofsky) fcfb952bca Improve TestNodeCLI output parsing (Russell Yanofsky) Pull request description: Lack of test coverage was pointed out by @jnewbery in bitcoin/bitcoin#11687 (comment) Tree-SHA512: 5f10e31abad11a5edab0da4e2515e39547adb6ab9e55e50427ab2eb7ec9a43d6b896b579b15863e5edc9beee7d8bf1c84d9dabd247be0760a1b9ae39e1e8ee02
2f3bd47 Abstract directory locking into util.cpp (MeshCollider) 5260a4a Make .walletlock distinct from .lock (MeshCollider) 64226de Generalise walletdir lock error message for correctness (MeshCollider) c9ed4bd Add a test for wallet directory locking (MeshCollider) e60cb99 Add a lock to the wallet directory (MeshCollider) Pull request description: Fixes bitcoin/bitcoin#11888, needs a 0.16 milestone Also adds a test that the lock works. bitcoin/bitcoin#11687 will probably rework this to a per-wallet lock instead of just the walletdir, but this fixes the current issue Tree-SHA512: 98e52d67f820e3b8f919cf361ffbb7d928f1bd67603e0ed26c5076ea02d9b3a90c3535ddf7329f3b88171396fa28dd3c87adab3577a8a217bd1e4247bda99138
be8ab7d08 Create new wallet databases as directories rather than files (Russell Yanofsky) 26c06f24e Allow wallet files not in -walletdir directory (Russell Yanofsky) d8a99f65e Allow wallet files in multiple directories (Russell Yanofsky) Pull request description: This change consists of three commits: * The first commit is a pure refactoring that removes the restriction that two wallets can only be opened at the same time if they are contained in the same directory. * The second commit removes the restriction that `-wallet` filenames can only refer to files in the `-walletdir` directory. * The third commit makes second commit a little safer by changing bitcoin to create wallet databases as directories rather than files, so they can be safely backed up. All three commits should be straightforward: * The first commit adds around 20 lines of new code and then updates a bunch of function signatures (generally updating them to take plain fs::path parameters, instead of combinations of strings, fs::paths, and objects like CDBEnv and CWalletDBWrapper). * The second commit removes two `-wallet` filename checks and adds some test cases to the multiwallet unit test. * The third commit just changes the mapping from specified wallet paths to bdb environment & data paths. --- **Note:** For anybody looking at this PR for the first time, I think you can skip the comments before _20 Nov_ and start reading at bitcoin/bitcoin#11687 (comment). Comments before _20 Nov_ were about an earlier version of the PR that didn't include the third commit, and then confusion from not seeing the first commit. Tree-SHA512: 00bbb120fe0df847cf57014f75f1f7f1f58b0b62fa0b3adab4560163ebdfe06ccdfff33b4231693f03c5dc23601cb41954a07bcea9a4919c8d42f7d62bcf6024
…param c1e5d40 Make debugging test crash easier (MeshCollider) 8263f6a Create walletdir if datadir doesn't exist and fix tests (MeshCollider) 9587a9c Default walletdir is wallets/ if it exists (MeshCollider) d987889 Add release notes for -walletdir and wallets/ dir (MeshCollider) 80c5cbc Add test for -walletdir (MeshCollider) 0530ba0 Add -walletdir parameter to specify custom wallet dir (MeshCollider) Pull request description: Closes bitcoin#11348 Adds a `-walletdir` parameter which specifies a directory to use for wallets, allowing them to be stored separately from the 'main' data directory. Creates a new `wallets/` directory in datadir if this is the first time running, and defaults to using it if it exists. Includes tests and release notes. Things which might need to be considered more: - there is no 'lock' on the wallets directory, which might be needed? - because this uses a new wallets/ directory by default, downgrading to an earlier version won't see the wallets in that directory (not a big deal though, users can just copy them up to the main dir) - jnewbery suggested putting each wallet in its own directory, which is a good idea, but out of scope for this PR IMO. EDIT: this is being done in bitcoin#11687 - doc/files.md needs updating (will do soon) I also considered including a cleanup by removing caching of data directory paths and instead just initialise them once on startup (c.f. dashpay#3073), but decided it wasn't super relevant here will just complicate review. Tree-SHA512: c8ac04bfe9a810c32055f2c8b8fa0d535e56125ceb8d96f12447dd3538bf3e5ee992b60b1cd2173bf5f3fa023a9feab12c9963593bf27ed419df929bb413398d
This change consists of three commits:
-wallet
filenames can only refer to files in the-walletdir
directory.All three commits should be straightforward:
The first commit adds around 20 lines of new code and then updates a bunch of function signatures (generally updating them to take plain fs::path parameters, instead of combinations of strings, fs::paths, and objects like CDBEnv and CWalletDBWrapper).
The second commit removes two
-wallet
filename checks and adds some test cases to the multiwallet unit test.The third commit just changes the mapping from specified wallet paths to bdb environment & data paths.
Note: For anybody looking at this PR for the first time, I think you can skip the comments before 20 Nov and start reading at #11687 (comment). Comments before 20 Nov were about an earlier version of the PR that didn't include the third commit, and then confusion from not seeing the first commit.