External wallet files #11687

pull ryanofsky wants to merge 3 commits into bitcoin:master from ryanofsky:pr/wfile changing 17 files +313 −207
  1. ryanofsky commented at 8:03 pm on November 14, 2017: member

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

  2. luke-jr commented at 8:05 pm on November 14, 2017: member
    This is too dangerous. It’s not clear at face value to users that the database/ directory will be created or must be maintained with the wallet file(s).
  3. gmaxwell commented at 8:44 pm on November 14, 2017: contributor
    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.
  4. meshcollider commented at 9:43 pm on November 14, 2017: contributor
    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.
  5. fanquake added the label Wallet on Nov 14, 2017
  6. meshcollider commented at 9:34 am on November 15, 2017: contributor
    Note that you might want to cherrypick https://github.com/bitcoin/bitcoin/pull/11466/commits/c36cb54711fe7677db6efc4db00a6e7d42e62f8d to help debug the travis failure
  7. laanwj commented at 4:50 pm on November 16, 2017: member

    FWIW, if you separate a wallet from its database directory you reliably get a wallet that won’t open.

    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.

  8. ryanofsky force-pushed on Nov 16, 2017
  9. ryanofsky force-pushed on Nov 17, 2017
  10. in src/bitcoin-cli.cpp:49 in beedd374f0 outdated 
    47@@ -48,7 +48,7 @@ std::string HelpMessageCli()
48     strUsage += HelpMessageOpt("-rpcclienttimeout=<n>", strprintf(_("Timeout in seconds during HTTP requests, or 0 for no timeout. (default: %d)"), DEFAULT_HTTP_CLIENT_TIMEOUT));
49     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.")));
50     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."));
51-    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)"));
52+    strUsage += HelpMessageOpt("-rpcwallet=<walletname>", _("Send RPC for non-default wallet on RPC server (needs to exactly match corresponding -wallet option passed to bitcoind)"));
    promag commented at 4:44 pm on November 17, 2017:
    Full path? How will the url look like?
    ryanofsky commented at 4:53 pm on November 17, 2017:

    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.

    promag commented at 5:19 pm on November 17, 2017:
    Feels weird to known server side details. Not to mention it can break the client if you move the wallet to a new place.
    ryanofsky commented at 5:27 pm on November 17, 2017:

    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.

    ryanofsky commented at 5:32 pm on November 17, 2017:

    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)

    promag commented at 5:45 pm on November 17, 2017:
    My point is that IMO we should not allow absolute path in rpcwallet, which you introduce here.
    ryanofsky commented at 5:53 pm on November 17, 2017:

    What is the problem exactly? If the server operator wants to use a plain path, or a relative path, he can use one. If he wants to use a symlink alias, he can use that. What is the problem with allowing him to use a full path as well?

    Note that RPC identifier for wallet is entirely up to the server and not up the client. The client has to use whatever string the server uses to identify the wallet, there is no translation of any kind done on the client value.

    promag commented at 6:22 pm on November 17, 2017:
    I’m not arguing against full paths server side. I’m kind of against exposing absolute paths to RPC clients. listwallets will expose the full path right?
    sipa commented at 6:28 pm on November 17, 2017:

    It exposes the name the server uses for the wallet.

    In the current implementation that name indeed corresponds to the configured path, but there is no reason why that must remain the case.

    promag commented at 6:33 pm on November 17, 2017:
    Well if the goal is to have some sort of id and have that exposed instead then +1.
    ryanofsky commented at 6:35 pm on November 17, 2017:

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

    promag commented at 6:50 pm on November 17, 2017:
    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..
    promag commented at 6:52 pm on November 17, 2017:
    My point is to not even allow such cases. An wallet id (filename or even relative path) is enough
    ryanofsky commented at 7:17 pm on November 17, 2017:

    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?

    promag commented at 7:45 pm on November 17, 2017:

    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.

  11. ryanofsky force-pushed on Nov 17, 2017
  12. ryanofsky commented at 6:53 pm on November 17, 2017: member

    This is too dangerous. It’s not clear at face value to users that the database/ directory will be created or must be maintained with the wallet file(s).

    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.

    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.

    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.

    Agree with @luke-jr, #11466 is safer until we decide on a better wallet directory structure (https://github.com/bitcoin/bitcoin/pull/11466#issuecomment-335827526), which would involve supporting multiple BDB environments first.

    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.

  13. meshcollider commented at 0:46 am on November 18, 2017: contributor

    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.

    Indeed, concept ACK on the new approach, will review soon

  14. laanwj referenced this in commit d080a7d503 on Nov 18, 2017
  15. luke-jr commented at 3:13 pm on November 18, 2017: member

    IMO we should still forbid paths for file-based wallets, and only allow it for directory-based ones.

    Suggest renaming wallet.dat in the directory to wallet.bdb to convey the fact that you can’t just copy the file to make a proper backup.

  16. sipa commented at 8:10 pm on November 18, 2017: member
    Concept ACK
  17. laanwj commented at 8:35 am on November 20, 2017: member

    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.

    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.

    Suggest renaming wallet.dat in the directory to wallet.bdb to convey the fact that you can’t just copy the file to make a proper backup.

    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.

  18. ryanofsky force-pushed on Nov 21, 2017
  19. ryanofsky force-pushed on Nov 22, 2017
  20. ryanofsky force-pushed on Nov 30, 2017
  21. ryanofsky commented at 7:11 pm on November 30, 2017: member

    IMO we should still forbid paths for file-based wallets, and only allow it for directory-based ones.

    Good idea, added check for this.

  22. in src/wallet/db.h:90 in 3dba60c313 outdated 
    85@@ -85,7 +86,8 @@ class CDBEnv
86     }
87 };
88 
89-extern CDBEnv bitdb;
90+/** Get CDBEnv and database filename given a wallet path. */
91+void GetWalletEnv(const fs::path& wallet_path, CDBEnv*& env, std::string& database_filename);
    TheBlueMatt commented at 10:20 pm on December 5, 2017:
    Ugh, can we return the CDBEnv* instead of return-by-setting-ref-to-ptr?
    ryanofsky commented at 9:07 pm on December 6, 2017:
    Ugh. Good idea! Done in 75ed8c595c53a2c3aeedfbc01810f291ee92ad51
  23. in src/wallet/db.h:108 in 3dba60c313 outdated 
    101@@ -100,9 +102,15 @@ class CWalletDBWrapper
102     }
103 
104     /** Create DB handle to real database */
105-    CWalletDBWrapper(CDBEnv *env_in, const std::string &strFile_in) :
106-        nUpdateCounter(0), nLastSeen(0), nLastFlushed(0), nLastWalletUpdate(0), env(env_in), strFile(strFile_in)
107+    CWalletDBWrapper(const fs::path& wallet_path, bool mock = false) :
108+        nUpdateCounter(0), nLastSeen(0), nLastFlushed(0), nLastWalletUpdate(0)
109     {
110+        GetWalletEnv(wallet_path, env, strFile);
    TheBlueMatt commented at 6:22 pm on December 6, 2017:
    Why create the env before checking for mock? Why not add a fMock parameter to GetWalletEnv or so instead?
    ryanofsky commented at 9:20 pm on December 6, 2017:
    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…
    TheBlueMatt commented at 4:39 pm on December 12, 2017:
    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.
    ryanofsky commented at 4:54 pm on December 12, 2017:

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

    TheBlueMatt commented at 9:33 pm on December 12, 2017:
    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.
  24. in src/wallet/db.h:38 in 3dba60c313 outdated 
    34@@ -35,17 +35,18 @@ class CDBEnv
35     void EnvShutdown();
36 
37 public:
38-    mutable CCriticalSection cs_db;
    TheBlueMatt commented at 8:20 pm on December 6, 2017:
    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.
    ryanofsky commented at 9:04 pm on December 6, 2017:
    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.
    TheBlueMatt commented at 4:43 pm on December 12, 2017:
    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).
  25. in src/wallet/db.cpp:105 in 3dba60c313 outdated 
    102+    Close();
103 }
104 
105 void CDBEnv::Close()
106 {
107+    for (auto& db : mapDb) {
    TheBlueMatt commented at 8:24 pm on December 6, 2017:
    While you’re at it, can you just merge EnvShutdown and Close? EnvShutdown is now only called from Close().
    ryanofsky commented at 8:58 pm on December 6, 2017:
    Done in 75ed8c595c53a2c3aeedfbc01810f291ee92ad51
  26. in src/wallet/db.cpp:503 in 3dba60c313 outdated 
    466@@ -435,7 +467,9 @@ CDB::CDB(CWalletDBWrapper& dbw, const char* pszMode, bool fFlushOnCloseIn) : pdb
467             if (ret != 0) {
468                 throw std::runtime_error(strprintf("CDB: Error %d, can't open database %s", ret, strFilename));
469             }
470-            CheckUniqueFileid(*env, strFilename, *pdb_temp);
471+            for (auto& env : g_dbenvs) {
    TheBlueMatt commented at 8:33 pm on December 6, 2017:
    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.
    ryanofsky commented at 8:58 pm on December 6, 2017:
    Added comment in 75ed8c595c53a2c3aeedfbc01810f291ee92ad51. It’s done to prevent opening two paths that refer to the same file at the same time.
    TheBlueMatt commented at 9:34 pm on December 12, 2017:
    Did you forget to push? Noting this is the only way we handle hardlinks is useful, indeed.
    ryanofsky commented at 10:06 pm on December 12, 2017:

    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.

  27. in src/wallet/wallet.cpp:3860 in 3dba60c313 outdated 
    3859     if (gArgs.GetBoolArg("-zapwallettxes", false)) {
3860         uiInterface.InitMessage(_("Zapping all transactions from wallet..."));
3861 
3862-        std::unique_ptr<CWalletDBWrapper> dbw(new CWalletDBWrapper(&bitdb, walletFile));
3863-        std::unique_ptr<CWallet> tempWallet = MakeUnique<CWallet>(std::move(dbw));
3864+        std::unique_ptr<CWallet> tempWallet = MakeUnique<CWallet>(name, path);
    TheBlueMatt commented at 8:38 pm on December 6, 2017:
    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).
    ryanofsky commented at 9:49 pm on December 6, 2017:

    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:

    0MakeUnique<CWallet>(name, path)
1MakeUnique<CWallet>(CWallet::Mock())
2MakeUnique<CWallet>(CWallet::Dummy()))

    would become something like:

    0MakeUnique<CWallet>(name, MakeUnique<CWalletDBWrapper>(path))
1MakeUnique<CWallet>("mock", MakeUnique<CWalletDBWrapper>(CWalletDBWrapper::Mock()))
2MakeUnique<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.

    TheBlueMatt commented at 4:29 pm on December 12, 2017:
    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.
    ryanofsky commented at 4:36 pm on December 12, 2017:

    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.

    TheBlueMatt commented at 9:30 pm on December 12, 2017:
    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?
    ryanofsky commented at 11:00 pm on December 12, 2017:
    Done in 39fa1e9c01417bc9275d8f81bfef67bfc5ca308e
  28. ryanofsky commented at 11:13 pm on December 6, 2017: member
    Added 2 commits 2cf4dc83328aae3ad120ca7b0a0dc39848346400 -> ec01271ca284a0ee73e58776cb3c651953c5536c (pr/wfile.7 -> pr/wfile.8, compare) Squashed ec01271ca284a0ee73e58776cb3c651953c5536c -> c25ed1fcecd31a5f4243205cdc43b645e2a8cdb6 (pr/wfile.8 -> pr/wfile.9)
  29. ryanofsky force-pushed on Dec 12, 2017
  30. ryanofsky commented at 11:01 pm on December 12, 2017: member
    Added 2 commits c25ed1fcecd31a5f4243205cdc43b645e2a8cdb6 -> b9440abd55804a6f001c58288cc7fd2124aa1007 (pr/wfile.9 -> pr/wfile.10, compare) Squashed b9440abd55804a6f001c58288cc7fd2124aa1007 -> 99934923d4c1dc4c0f96865b2589cc0b5df66b31 (pr/wfile.10 -> pr/wfile.11)
  31. ryanofsky force-pushed on Dec 12, 2017
  32. in src/wallet/db.h:104 in 99934923d4 outdated 
    100@@ -100,9 +101,33 @@ class CWalletDBWrapper
101     }
102 
103     /** Create DB handle to real database */
104-    CWalletDBWrapper(CDBEnv *env_in, const std::string &strFile_in) :
105-        nUpdateCounter(0), nLastSeen(0), nLastFlushed(0), nLastWalletUpdate(0), env(env_in), strFile(strFile_in)
106+    CWalletDBWrapper(const fs::path& wallet_path, bool mock = false) :
    TheBlueMatt commented at 8:52 pm on December 13, 2017:
    Make the constructors private now?
    ryanofsky commented at 7:47 pm on December 15, 2017:

    Make the constructors private now?

    I didn’t do this just because it prevents being able to call MakeUnique so makes code below more verbose. But I think I’ll wind up making this tweak if @promag or someone else asks for other style changes on the PR.

  33. TheBlueMatt commented at 10:20 pm on December 13, 2017: member
    utACK 99934923d4c1dc4c0f96865b2589cc0b5df66b31. Is a good improvement, but this stuff is quite sticky, so probably needs some additional pretty-through review.
  34. laanwj added this to the "Blockers" column in a project
  35. in src/wallet/db.cpp:135 in 8b036a198b outdated 
    125         return true;
126 
127     boost::this_thread::interruption_point();
128 
129-    strPath = pathIn.string();
130+    fs::path pathIn = strPath;
    PierreRochard commented at 11:53 pm on December 16, 2017:
    Lines 32:33 in db.h states // Don't change into fs::path, as that can result in // shutdown problems/crashes caused by a static initialized internal pointer. When originally written, this comment referred to boost::filesystem. I’m not sure if this comment is still relevant but I did want to bring attention to it.
    ryanofsky commented at 4:08 pm on December 18, 2017:

    When originally written, this comment referred to boost::filesystem.

    The comment is actually still referring to boost::filesystem, fs is just an alias: https://github.com/bitcoin/bitcoin/blob/62fdf9b07087b80d2142799bdd2324f61483359d/src/fs.h#L16

    But it’s fine to create a path object here. The comment is just explaining why the path objects aren’t stored permanently (they stop being usable during shutdown).

  36. in src/wallet/db.cpp:80 in 8b036a198b outdated 
    61+CDBEnv* GetWalletEnv(const fs::path& wallet_path, std::string& database_filename)
62+{
63+    fs::path env_directory = wallet_path.parent_path();
64+    database_filename = wallet_path.filename().string();
65+    LOCK(cs_db);
66+    return &g_dbenvs.emplace(std::piecewise_construct, std::forward_as_tuple(env_directory.string()), std::forward_as_tuple(env_directory)).first->second;
    PierreRochard commented at 0:11 am on December 17, 2017:
    style: tuple unpacking in the return statement reduces readability
    ryanofsky commented at 4:04 pm on December 18, 2017:

    style: tuple unpacking in the return statement reduces readability

    Unclear what preferred alternative is, but also this is replaced in aca8ec0dabb4987a4f73a19e2a7dc09177197adb from #11911 so it might not be relevant for long.

  37. meshcollider commented at 10:12 pm on December 18, 2017: contributor

    utACK https://github.com/bitcoin/bitcoin/pull/11687/commits/99934923d4c1dc4c0f96865b2589cc0b5df66b31

    This is still on my to-do list for testing as well

  38. in src/wallet/db.h:92 in 99934923d4 outdated 
    84@@ -85,7 +85,8 @@ class CDBEnv
85     }
86 };
87 
88-extern CDBEnv bitdb;
89+/** Get CDBEnv and database filename given a wallet path. */
90+CDBEnv* GetWalletEnv(const fs::path& wallet_path, std::string& database_filename);
91 
92 /** An instance of this class represents one database.
93  * For BerkeleyDB this is just a (env, strFile) tuple.
    jnewbery commented at 5:28 pm on December 20, 2017:
    I think this comment is now outdated. CWalletDBWrapper was previously initialized with the CDBEnv and strFile. This is no longer the case. Suggest you just remove this line.
    ryanofsky commented at 10:01 pm on December 20, 2017:

    #11687 (review)

    The comment is still correct. CWalletDBWrapper class members haven’t changed at all and the object still just holds an env pointer and a filename. There are comments below on how the class is initialized but these are on the constructors rather than the class itself. I think this class comment is helpful, so I’d prefer to keep it. Renaming PR #11851 might also make some of this clearer, too.

    jnewbery commented at 6:59 pm on January 9, 2018:

    #11687 (review)

    Yes, agree.

  39. in src/wallet/db.cpp:318 in 99934923d4 outdated 
    286@@ -258,8 +287,12 @@ bool CDB::Recover(const std::string& filename, void *callbackDataIn, bool (*reco
287     return fSuccess;
288 }
289 
290-bool CDB::VerifyEnvironment(const std::string& walletFile, const fs::path& walletDir, std::string& errorStr)
291+bool CDB::VerifyEnvironment(const fs::path& file_path, std::string& errorStr)
292 {
293+    std::string walletFile;
    jnewbery commented at 7:19 pm on December 20, 2017:

    sorry for nitting, but this should be filename (like the function above, in lowercase (or snake case if you want to make it two words))

    Same for a few other variable names in this file.

    Scrap that - I see that you’re maintaining the same variable naming to minimize code churn.

  40. in src/wallet/init.cpp:69 in 99934923d4 outdated 
    62@@ -63,7 +63,7 @@ bool WalletParameterInteraction()
63         return true;
64     }
65 
66-    gArgs.SoftSetArg("-wallet", DEFAULT_WALLET_DAT);
67+    gArgs.SoftSetArg("-wallet", "");
    jnewbery commented at 8:25 pm on December 20, 2017:

    This means that m_name is "" if no -wallet name is specified on the command line or in the .conf file, which makes the getwalletinfo and listwallets output confusing:

     0bitcoin-cli getwalletinfo
 1{
 2  "walletname": "",
 3  "walletversion": 159900,
 4  "balance": 0.00000000,
 5  "unconfirmed_balance": 0.00000000,
 6  "immature_balance": 0.00000000,
 7  "txcount": 0,
 8  "keypoololdest": 1513800503,
 9  "keypoolsize": 1000,
10  "keypoolsize_hd_internal": 1000,
11  "paytxfee": 0.00000000,
12  "hdmasterkeyid": "4a4cbfcac33849b565a2eb5deecbc96c5c76c06a"
13}
14
15→ bitcoin-cli listwallets
16[
17  ""
18]

    Is it possible to change this so m_name is filled to wallet.dat when no wallet parameter is passed?

    ryanofsky commented at 10:16 pm on December 20, 2017:

    #11687 (review)

    Is it possible to change this so m_name is filled to wallet.dat when no wallet parameter is passed?

    Added a note about getwalletinfo/listwallets to release notes in 8e7981fe35f359fdbddd92f468443a61c844f392.

    The current behavior is if you don’t specify a -wallet option, then a default wallet is created for you that has no name. If you do specify a -wallet option, whatever value you pass will be the wallet name.

    I think listing wallet.dat as the default name would not be good. Even though specifying -wallet=wallet.dat (or any other filename) is still allowed for backwards compatibility, the idea here is that new wallets are directories, not files, and the wallet.dat filename is a implementation detail, just like the names of other files in wallet directories (db.log, database/log.0000000001).

  41. in test/functional/multiwallet.py:48 in 99934923d4 outdated 
    42+        #   w          - to verify wallet name matching works when one wallet path is prefix of another
43+        #   sub/w5     - to verify relative wallet path is created correctly
44+        #   extern/w6  - to verify absolute wallet path is created correctly
45+        #   w7_symlink - to verify symlinked wallet path is initialized correctly
46+        #   w8         - to verify existing wallet file is loaded correctly
47+        #   ''         - to verify default wallet file is created correctly
    jnewbery commented at 8:57 pm on December 20, 2017:

    I don’t think there’s any way to access the ‘default’ wallet with name '' through the CLI.

    0(Pdb) self.nodes[0].cli("--rpcwallet=").getwalletinfo()
1*** subprocess.CalledProcessError: Command 'bitcoin-cli' returned non-zero exit status 18
    ryanofsky commented at 10:23 pm on December 20, 2017:

    #11687 (review)

    I don’t think there’s any way to access the ‘default’ wallet with name ’’ through the CLI.

    This is good catch, and fixed in d273391b30e35a5bfcb53403936662c690ec205f. It was possible to work around this by starting bitcoin with -wallet=wallet.dat, which is what you need to before this PR anyway, but better to fix this. I also made #11970 which extends multiwallet test to cover bitcoin-cli, and would have caught this bug.

  42. in test/functional/multiwallet.py:114 in 99934923d4 outdated 
    121+        # check wallet names and balances
122+        wallets[0].generate(1)
123+        for wallet_name, wallet in zip(wallet_names, wallets):
124+            info = wallet.getwalletinfo()
125+            assert_equal(info['immature_balance'], 50 if wallet is wallets[0] else 0)
126+            assert_equal(info['walletname'], wallet_name)
    jnewbery commented at 8:58 pm on December 20, 2017:

    Can you also add CLI testing to this test, to verify that the -rpcwallet argument still works? Something like:

    0self.nodes[0].cli("--rpcwallet={}".format(wallet_name)).getwalletinfo()
    ryanofsky commented at 10:23 pm on December 20, 2017:

    #11687 (review)

    Can you also add CLI testing to this test

    Added simple test in d273391b30e35a5bfcb53403936662c690ec205f, more comprehensive tests in #11970.

  43. jnewbery commented at 8:58 pm on December 20, 2017: member
    Looks great. A few minor questions/suggestions inline.
  44. sipa commented at 11:17 pm on December 20, 2017: member

    Code changes look good, and some of the refactors really help with readability.

    Two things I’m unsure about:

    • Making wallets directories. Maybe it results in slightly easier backup procedures (if people interpret it correctly), but it’s also more mental load (“my wallet is just wallet.dat right?”).
    • Changing the wallet dir based on whether $DATADIR/wallets exists, that seems a bit fragile. Though it seems that’s already in master?
  45. meshcollider commented at 0:08 am on December 21, 2017: contributor
    @sipa yep that was part of #11466
  46. ryanofsky force-pushed on Dec 21, 2017
  47. ryanofsky commented at 12:46 pm on December 21, 2017: member
    Rebased 99934923d4c1dc4c0f96865b2589cc0b5df66b31 -> 5c0d5c783ce97a2013219989bf1c6dd105916878 (pr/wfile.11 -> pr/wfile.12) due to conflict in release notes. Added 2 commits 5c0d5c783ce97a2013219989bf1c6dd105916878 -> 8e7981fe35f359fdbddd92f468443a61c844f392 (pr/wfile.12 -> pr/wfile.13, compare) Squashed 8e7981fe35f359fdbddd92f468443a61c844f392 -> fbc2402357dedd1e97cb2adcaef452393e656e8c (pr/wfile.13 -> pr/wfile.14)
  48. ryanofsky commented at 1:14 pm on December 21, 2017: member

    Two things I’m unsure about:

    • Making wallets directories. Maybe it results in slightly easier backup procedures (if people interpret it correctly), but it’s also more mental load (“my wallet is just wallet.dat right?”).

    I’m not sure I understand the point about mental load.

    For typical user who isn’t using multiwallet or setting any -wallet=path option, nothing is changing here. wallet.dat is still created in the same place as before and can be backed up the same way. For more advanced users who do specify -wallet=<path> options, the specified path will just be created as a directory instead of a file.

    • Changing the wallet dir based on whether $DATADIR/wallets exists, that seems a bit fragile. Though it seems that’s already in master?

    Yes, this was done in #11466. I think a less fragile implementation of the idea might search for existing wallets in both <walletdir> and <datadir> but create all new wallets in <walletdir>. This could be implemented entirely in the wallet and would avoid the init order dependencies introduced in #11466 between node, qt, and wallet code. However, I don’t actually understand what was accomplished in #11466 by changing the default wallet directory to begin with, so maybe this falls short in some way. Probably better to discuss somewhere else like #11466, though, since it’s not really related to this PR.

  49. in src/bitcoin-cli.cpp:343 in fbc2402357 outdated 
    337@@ -338,8 +338,8 @@ static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, co
338 
339     // check if we should use a special wallet endpoint
340     std::string endpoint = "/";
341-    std::string walletName = gArgs.GetArg("-rpcwallet", "");
342-    if (!walletName.empty()) {
343+    if (!gArgs.GetArgs("-rpcwallet").empty()) {
344+        std::string walletName = gArgs.GetArg("-rpcwallet", "");
    promag commented at 3:27 pm on December 21, 2017:
    Why this change? To reduce walletName scope?
    ryanofsky commented at 3:56 pm on December 21, 2017:

    Why this change? To reduce walletName scope?

    Yes and to avoid declaring a variable that will never be used.

  50. in test/functional/conf_args.py:40 in b355cf7750 outdated 
    36@@ -37,13 +37,13 @@ def run_test(self):
37         os.mkdir(new_data_dir)
38         self.start_node(0, ['-conf='+conf_file, '-wallet=w1'])
39         self.stop_node(0)
40-        assert os.path.isfile(os.path.join(new_data_dir, 'regtest', 'wallets', 'w1'))
41+        assert os.path.exists(os.path.join(new_data_dir, 'regtest', 'wallets', 'w1'))
    promag commented at 3:43 pm on December 21, 2017:
    isdir?
    ryanofsky commented at 3:56 pm on December 21, 2017:
    Seems worse, makes test more fragile than it needs to be.
    promag commented at 2:50 am on December 29, 2017:
    Yeah, agree, not relevant here.
  51. promag commented at 3:44 pm on December 21, 2017: member
    Testing.
  52. ryanofsky commented at 5:19 pm on December 21, 2017: member
    Added 1 commit fbc2402357dedd1e97cb2adcaef452393e656e8c -> b355cf77508c5b2086062f151d83ff09b1371ca9 (pr/wfile.14 -> pr/wfile.15, compare) to fix travis test. Squashed b355cf77508c5b2086062f151d83ff09b1371ca9 -> 5e8c79c1e8754bca52d7185983ba69dc9a33102f (pr/wfile.15 -> pr/wfile.16)
  53. ryanofsky force-pushed on Dec 21, 2017
  54. ryanofsky force-pushed on Dec 21, 2017
  55. sipa commented at 2:44 pm on December 25, 2017: member

    @ryanofsky Removed my concern about the wallets-becoming-directories. I guess that anyone who isn’t aware that wallets can be directories will in fact not have any such wallets, as requires explicitly configuring a directory.

    Needs rebase.

  56. promag commented at 2:55 am on December 29, 2017: member
    @ryanofsky needs rebase.
  57. ryanofsky force-pushed on Dec 31, 2017
  58. ryanofsky commented at 9:43 pm on December 31, 2017: member
    Rebased 5e8c79c1e8754bca52d7185983ba69dc9a33102f -> 2fd9c2b318b69ce3e7ce1fba5eb7f3fb5c781876 (pr/wfile.16 -> pr/wfile.17) to avoid merge conflict in release notes. No changes.
  59. ryanofsky commented at 12:46 pm on January 4, 2018: member

    Status of this PR is unclear, but more review is welcome. So far have:

    Code ack MeshCollider Dec 18 #11687 (comment) Code ack from TheBlueMatt Dec 13 #11687#pullrequestreview-83313824 Concept ack from sipa Nov 18 #11687 (comment) and Dec 25 #11687 (comment) Concept ack from laanwj Nov 20 #11687 (comment) Partial reviews from jnewbery, promag, and PierreRochard.

    There is a trivial merge conflict with segwit wallet #11403 in wallet_test_fixture.cpp, but this could probably be avoided if it matters.

  60. jnewbery commented at 0:03 am on January 9, 2018: member

    Sorry for the late concept feedback, but the config model seems a little confusing to me:

    • datadir defaults to ~/.bitcoin (on linux) and can be overridden by an absolute or relative path (relative to cwd)
    • walletdir detaults to <datadir>/wallets if it exists, or <datadir> if it doesn’t exist, and can be overridden by an absolute or relative path (relative to cwd)
    • wallet defaults to <walletdir> and can be overridden by an absolute or relative path (relative to <walletdir>).

    Without any arguments, a wallet will be created at <datadir>/wallets/wallet.dat (if <datadir>/wallets exists) or <datadir>/wallet.dat (if <datadir>/wallets does not exist). listwallets and getwalletinfo will show this wallet’s name to be ''. If bitcoind is restarted with --wallet=wallet.dat, listwallets and getwalletinfo will show the same wallet’s name to be wallet.dat.

    The main points of confusion:

    1. datadir and walletdir paths are relative to cwd, but wallet is relative to walletdir
    2. walletdir default depends on existence of wallets directory in datadir
    3. wallet name can depend on how bitcoind was invoked

    I think (1) and (2) could be addressed with very clear documentation (I think the release notes are almost there, but the help text could also be improved). You could also consider disallowing relative paths for wallet - it could either take a string (which would refer to a .dat file or directory in walletdir) or an absolute path.

    I don’t like (3) and think that the wallet name should always exactly equal the wallet file or directory.

  61. in src/wallet/db.cpp:80 in 2fd9c2b318 outdated 
    71+        // data and log files.
72+        env_directory = wallet_path;
73+        database_filename = "wallet.dat";
74+    }
75+    LOCK(cs_db);
76+    return &g_dbenvs.emplace(std::piecewise_construct, std::forward_as_tuple(env_directory.string()), std::forward_as_tuple(env_directory)).first->second;
    jnewbery commented at 7:51 pm on January 9, 2018:

    Note that std::map::emplace can construct the object even if the key exists in the map, according to http://en.cppreference.com/w/cpp/container/map/emplace (“The element may be constructed even if there already is an element with the key in the container, in which case the newly constructed element will be destroyed immediately.”)

    In this case I think it’s fine - the constructor simply calls Reset() and then the destructor gets called.

    This goes away in the follow-up PR (#11911), but perhaps just add a comment explaining this.

    ryanofsky commented at 3:07 pm on January 10, 2018:

    Note that std::map::emplace can construct the object even if the key exists in the map

    Added comment in 69de7c1d8d8326d159634e8949bc3ac67266f534.

    I was surprised by this but it actually does seem to happen in practice. Following test program shows two objects being constructed:

     0#include <map>
 1#include <iostream>
 2
 3using namespace std;
 4
 5struct C {
 6  C(int) {cout << " C " << this << endl;}
 7  ~C() {cout << "~C " << this << endl;}
 8};
 9
10int main(int, char**) {
11  map<string, C> m;
12  m.emplace(piecewise_construct, forward_as_tuple("key"), forward_as_tuple(5));
13  cout << "-" << endl;
14  m.emplace(piecewise_construct, forward_as_tuple("key"), forward_as_tuple(5));
15  cout << "-" << endl;
16}
  62. in src/wallet/init.cpp:36 in 2fd9c2b318 outdated 
    32@@ -33,7 +33,7 @@ std::string GetWalletHelpString(bool showDebug)
33     strUsage += HelpMessageOpt("-txconfirmtarget=<n>", strprintf(_("If paytxfee is not set, include enough fee so transactions begin confirmation on average within n blocks (default: %u)"), DEFAULT_TX_CONFIRM_TARGET));
34     strUsage += HelpMessageOpt("-walletrbf", strprintf(_("Send transactions with full-RBF opt-in enabled (RPC only, default: %u)"), DEFAULT_WALLET_RBF));
35     strUsage += HelpMessageOpt("-upgradewallet", _("Upgrade wallet to latest format on startup"));
36-    strUsage += HelpMessageOpt("-wallet=<file>", _("Specify wallet file (within data directory)") + " " + strprintf(_("(default: %s)"), DEFAULT_WALLET_DAT));
37+    strUsage += HelpMessageOpt("-wallet=<path>", _("Specify wallet database path. Can be specified multiple times to load multiple wallets. Path is interpreted relative to the bitcoin data directory if it is not absolute, and will be created if it does not exist (as a directory containing a wallet.dat file and log files). For backwards compatibility this will also accept names of existing data files in -walletdir.)"));
    jnewbery commented at 8:40 pm on January 9, 2018:
    I believe this help text is incorrect. Should be "Path is interpreted relative to the <walletdir> directory if it is not absolute"
    ryanofsky commented at 3:08 pm on January 10, 2018:

    I believe this help text is incorrect.

    Good catch! Fixed in 69de7c1d8d8326d159634e8949bc3ac67266f534. This was written before the -walletdir option was added.

  63. in test/functional/multiwallet.py:99 in 2fd9c2b318 outdated 
    111         assert_equal(w5_info['immature_balance'], 50)
112 
113         self.stop_node(0)
114 
115-        self.start_node(0, self.extra_args[0])
116+        self.start_node(0, extra_args)
    jnewbery commented at 8:53 pm on January 9, 2018:
    nit: can combine with line above for self.restart_node(0, extra_args)
    ryanofsky commented at 3:08 pm on January 10, 2018:

    nit: can combine with line above for self.restart_node(0, extra_args)

    Combined in 69de7c1d8d8326d159634e8949bc3ac67266f534.

  64. jnewbery commented at 9:03 pm on January 9, 2018: member

    Tested ACK 2fd9c2b318b69ce3e7ce1fba5eb7f3fb5c781876.

    This looks mostly good to me. A couple of comments inline. My only qualms are around the strange default wallet name '' and perhaps slightly better documentation of the interactions between the various arguments.

  65. ryanofsky commented at 6:33 pm on January 10, 2018: member

    Thanks for the review and feedback!

    Added 1 commits 2fd9c2b318b69ce3e7ce1fba5eb7f3fb5c781876 -> 69de7c1d8d8326d159634e8949bc3ac67266f534 (pr/wfile.17 -> pr/wfile.18, compare) Squashed 69de7c1d8d8326d159634e8949bc3ac67266f534 -> f9e7244449ad69b8b58bff4298bfacd9f16a1cd5 (pr/wfile.18 -> pr/wfile.19)

    Sorry for the late concept feedback, but the config model seems a little confusing to me:

    • datadir defaults to ~/.bitcoin (on linux) and can be overridden by an absolute or relative path (relative to cwd)
    • walletdir detaults to <datadir>/wallets if it exists, or <datadir> if it doesn’t exist, and can be overridden by an absolute or relative path (relative to cwd)
    • wallet defaults to <walletdir> and can be overridden by an absolute or relative path (relative to <walletdir>).

    Without any arguments, a wallet will be created at <datadir>/wallets/wallet.dat (if <datadir>/wallets exists) or <datadir>/wallet.dat (if <datadir>/wallets does not exist).

    Everything described above was done in #11466. I definitely think some of the rough edges of that PR could be smoothed off, and have some ideas of how to simplify it, but this PR just takes the walletdir as is and doesn’t change anything about how it is determined.

    listwallets and getwalletinfo will show this wallet’s name to be ''. If bitcoind is restarted with --wallet=wallet.dat, listwallets and getwalletinfo will show the same wallet’s name to be wallet.dat.

    Wallet naming is simple, I think. If you specify a -wallet=FOO option, the wallet name is FOO. If you don’t specify a wallet option, the wallet name is the empty string. Whatever wallet name you specify is used verbatim everywhere.

    The main points of confusion:

    1. datadir and walletdir paths are relative to cwd, but wallet is relative to walletdir
    2. walletdir default depends on existence of wallets directory in datadir
    3. wallet name can depend on how bitcoind was invoked

    I think (1) and (2) could be addressed with very clear documentation (I think the release notes are almost there, but the help text could also be improved). You could also consider disallowing relative paths for wallet - it could either take a string (which would refer to a .dat file or directory in walletdir) or an absolute path.

    I’m happy to write more documentation to address (1) and (2). I also think the behavior introduced in #11466 could be simplified, but that’s seems more like a topic to discuss in #11466 or somewhere else.

    I don’t like (3) and think that the wallet name should always exactly equal the wallet file or directory.

    I think the wallet name should simply be whatever string the user specifies. That was the case before this PR and is still the case now. Other more complicated approaches are possible, and I wouldn’t necessarily object to them, but it’s not clear from your feedback here exactly what you have in mind.

  66. ryanofsky force-pushed on Jan 11, 2018
  67. ryanofsky commented at 1:14 pm on January 11, 2018: member
    Rebased f9e7244449ad69b8b58bff4298bfacd9f16a1cd5 -> 0a2d86f8ef64927c444dadcfc1f700705af784fa (pr/wfile.19 -> pr/wfile.20) due to minor wallet_test_fixture.cpp conflict with #11403
  68. ryanofsky force-pushed on Jan 11, 2018
  69. ryanofsky commented at 4:00 pm on January 11, 2018: member
    Rebased 0a2d86f8ef64927c444dadcfc1f700705af784fa -> 5c8398dc03167f85ce5c8454fad8420dbcb89edb (pr/wfile.20 -> pr/wfile.21) due to minor conflict with #12150
  70. jnewbery commented at 10:11 pm on January 11, 2018: member

    As discussed, I think that the release notes in https://github.com/jnewbery/bitcoin/tree/pr11687.1 are perhaps clearer. I still think that the default '' name is a little odd, but with those updated release notes, I think it makes sense.

    ACK 5c8398dc03167f85ce5c8454fad8420dbcb89edb

  71. jnewbery commented at 10:12 pm on January 11, 2018: member
    This now has three code ACKs and two concept ACKs. It’d be nice to get this into v0.16 if at all possible so we don’t have two consecutive releases that change the wallet command line arguments.
  72. ryanofsky commented at 10:58 pm on January 11, 2018: member

    It’d be nice to get this into v0.16 if at all possible so we don’t have two consecutive releases that change the wallet command line arguments.

    Of course I’d be happier to see this merged sooner, but I also don’t think it’s a big deal if this is pushed back. Unlike #11466, this doesn’t change the location where files are created by default, so it’s more backwards compatible than that change was. And despite some confusion in the comments above, I still see change and #11466 as basically orthogonal, since #11466 is more about where wallets are located, and this PR is more about how they are formatted.

  73. alphaaurigae commented at 1:30 pm on January 12, 2018: none
    great, was exactly looking to have a wallet on a seperate luks drive.
  74. ryanofsky commented at 2:44 pm on January 12, 2018: member

    great, was exactly looking to have a wallet on a seperate luks drive.

    This will be possible for you either with or without this PR in 0.16, because #11466, which is already merged, lets you specify a -walletdir=<path> option with the path to a directory or directory symlink. The directory can hold one or more bitcoin wallets loaded in a shared berkeleydb environment just like -datadir currently. #11466 also added the ability to create a <datadir>/wallets symlink instead of specifying a -walletdir option.

    This PR extends #11466. It allows individual -wallet=<name> options to be paths, so if you have multiple wallets, they don’t all have to live in the same directory and share a common berkeley db environment. It also creates new wallets (both inside and outside of <walletdir>) as directories instead of files, so they can be backed up independently and do not have mixed data logs.

  75. alphaaurigae commented at 4:56 pm on January 12, 2018: none
    Got it running already - works perfecty and my IO problems solved - so nice :)
  76. MarcoFalke referenced this in commit b7450cdbd8 on Jan 12, 2018
  77. ryanofsky force-pushed on Jan 12, 2018
  78. ryanofsky commented at 11:02 pm on January 12, 2018: member
    Rebased 5c8398dc03167f85ce5c8454fad8420dbcb89edb -> 0797df3d2c06b132a911bed2e465834452f78d04 (pr/wfile.21 -> pr/wfile.22) due to conflicts with #11970 in multiwallet.py test. No other changes.
  79. jnewbery commented at 3:15 pm on January 15, 2018: member
    Tested ACK 0797df3d2c06b132a911bed2e465834452f78d04
  80. Sjors commented at 4:44 pm on January 15, 2018: member

    It’s quite useful to be able to keep the blockchain and wallet(s) in different locations. Using wallet directories also saves me trouble of remembering to type .dat.

    Tested 0797df3d. Code changes look sane, but are a little over my head.

    One surprising bit of behavior though is the difference between:

    0src/qt/bitcoin-qt -regtest -wallet=~/wallet
1src/qt/bitcoin-qt -regtest -wallet=~/wallet.dat

    The first command on OSX creates a /home/Username/wallet directory. The second creates a directory ~ inside the default regtest wallet directory.

    Maybe we should disallow a file extension for new wallets?

  81. ryanofsky commented at 6:10 pm on January 15, 2018: member

    The first command on OSX creates a /home/Username/wallet directory. The second creates a directory ~ inside the default regtest wallet directory.

    Interesting, what appears to be happening in that example is that your shell is expanding the tilde in the first command, but not expanding the tilde in your second command. (The PR doesn’t implement any tilde expansion itself.) Testing locally with bash versions 4.4.12 and 4.3.48, I don’t see tildes expanded in either of these cases. Bash documentation says that it only expands tildes at the beginning of words.

    In any case, apart from the inconsistent tilde expansion, the behaviour you are describing doesn’t actually seem surprising to me. If you specify a new path, and bitcoin goes and creates a directory at that path, this seems like pretty much the least surprising thing you could expect. But perhaps instead of allowing both absolute paths and paths relative to <walletdir> generally, the implementation could be restricted to only allow either absolute paths or paths directly in <walletdir>. Other options would be to forbid special characters or dots in paths like you suggested. I tend to be somewhat skeptical of approaches like these. It’s annoying when software you are trying to use has random and unnecessary restrictions on what inputs it accepts. But if there’s a realistic use-case where current behaviour would be harmful or confusing, adding some restrictions could make sense. For example, luke suggested earlier (https://github.com/bitcoin/bitcoin/pull/11687#issuecomment-345448447) that paths to freestanding data files (as opposed to directories) not in <walletdir> could be dangerous, so these are explicitly forbidden.

  82. laanwj referenced this in commit 66e3af709d on Jan 16, 2018
  83. ryanofsky force-pushed on Jan 16, 2018
  84. ryanofsky commented at 6:39 pm on January 17, 2018: member
    Rebased 0797df3d2c06b132a911bed2e465834452f78d04 -> 8a79f09a3dbcf48cfd8288ccf1d6bc3238aa1420 (pr/wfile.22 -> pr/wfile.23) after #11904 merge. Resolved minor conflicts in CDBEnv::Open, no other changes.
  85. Sjors commented at 8:29 am on January 18, 2018: member

    Can you update the PR description reference from -datadir to -walletdir?

    the implementation could be restricted to only allow either absolute paths or paths directly in <walletdir>

    Inclined to agree. See also discussion in #12166 (review) about relative paths.

    As you said, disallowing special characters doesn’t seem like a good idea. Except perhaps disallowing .dat specifically for new wallets, or at least throwing a warning.

  86. ryanofsky force-pushed on Jan 18, 2018
  87. ryanofsky commented at 4:02 pm on January 18, 2018: member

    Rebased 8a79f09a3dbcf48cfd8288ccf1d6bc3238aa1420 -> 2f87a02746b6afee5a21e647c5b3159172736bbb (pr/wfile.23 -> pr/wfile.24) due to conflict in release notes with #12210.

    Can you update the PR description reference from -datadir to -walletdir?

    Good catch! Done.

    the implementation could be restricted to only allow either absolute paths or paths directly in <walletdir>

    Inclined to agree. See also discussion in #12166 (review) about relative paths.

    Ok, I implemented this in 8c014825a6dba47f5a262da05e26394ef95b60df, but I didn’t add the change this PR because I don’t really like the complexity it adds and am not sure it is solving a real problem. To be clear, in the thread you linked to I was arguing that paths relative to $PWD were bad because they are unstable. I don’t think think paths relative to walletdir really have the same problem. I’m going to open a new PR to address the relative -walletdir issue today, and I think this should also address the inconsistency John was concerned about.

    As you said, disallowing special characters doesn’t seem like a good idea. Except perhaps disallowing .dat specifically for new wallets, or at least throwing a warning.

    I’m not sure what the warning would be. It’s not really clear to me what harm this would be trying to prevent.

  88. Sjors commented at 4:26 pm on January 18, 2018: member

    It’s not really clear to me what harm this would be trying to prevent.

    Accidentally creating new empty wallets.

  89. ryanofsky commented at 4:29 pm on January 18, 2018: member

    Accidentally creating new empty wallets.

    How exactly? If you specify the name of an existing file, it will just open the file. Also, again I’d be curious to know what would be a good warning.

  90. Sjors commented at 4:57 pm on January 18, 2018: member

    People will accidentally use .dat for wallets that are directories and forget .dat for wallets that are files. They’ll probably also create directories with .dat extension (which isn’t an error, but it’s not pretty).

    For this specific issue I would look for [path].dat if path doesn’t end with .dat and look for [path - .dat] if it does. A warning could be “Created a new wallet [path], but also found [path].dat”.

    Even better, but that would be breaking change, is to throw an error for non-existing wallets and introduce a new launch argument -createwallet (which doesn’t have to take an argument).

  91. ryanofsky commented at 5:11 pm on January 18, 2018: member
    Maybe we can discuss this in IRC, but a new wallet being created if you are trying to open a .dat file and forget the extension is something that happens today, and would always happen previously, unrelated to this PR. I think the changes you are talking about could be good idea but definitely belong in their own separate PR. It also seems to me this PR makes these problems less likely to occur going forward, because it allows wallets to be referenced as directories instead of as individual data files in a shared environment.
  92. laanwj referenced this in commit e839d6570d on Jan 18, 2018
  93. ryanofsky force-pushed on Jan 18, 2018
  94. Sjors commented at 8:47 am on January 19, 2018: member

    It also seems to me this PR makes these problems less likely to occur going forward, because it allows wallets to be referenced as directories

    For new users I would expect this to be the case. But I would expect existing users, who are used to adding “.dat” to a -wallet argument, to make mistakes, if they don’t read the release notes.

    I’m fine with leaving the PR as is. The release note are pretty clear.

  95. jnewbery commented at 9:17 pm on January 22, 2018: member
    retested ACK ec4e3820a8a2351ba26c88dcc02d99e29bfe5524. Changes are just the minimum required for rebases.
  96. ryanofsky force-pushed on Jan 25, 2018
  97. ryanofsky force-pushed on Feb 2, 2018
  98. ryanofsky force-pushed on Feb 5, 2018
  99. in src/wallet/wallet.h:21 in 1fe5528f4d outdated 
    17@@ -18,6 +18,7 @@
18 #include <wallet/crypter.h>
19 #include <wallet/walletdb.h>
20 #include <wallet/rpcwallet.h>
21+#include <util.h>
    promag commented at 9:28 am on February 6, 2018:
    Nit, sort :trollface:
    ryanofsky commented at 8:20 pm on February 14, 2018:

    Nit, sort :trollface:

    Thanks, done in https://github.com/bitcoin/bitcoin/commit/c8c87343fcc7d86e9bec100a609a521c6c1e2d34

  100. ryanofsky commented at 8:47 pm on February 12, 2018: member
    Rebased 2f87a02746b6afee5a21e647c5b3159172736bbb -> ec4e3820a8a2351ba26c88dcc02d99e29bfe5524 (pr/wfile.24 -> pr/wfile.25) due to release notes conflict with #12166. Rebased ec4e3820a8a2351ba26c88dcc02d99e29bfe5524 -> aff4b44f0da4c583b76d8e3db926caf3fa03d3c9 (pr/wfile.25 -> pr/wfile.26) due to release notes conflict with 0.16 version bump. Rebased d7daf096830d17c4c20952ac5b487b49d43b4d80 -> 1fe5528f4dea8b530022b9bf359426f2079848af (pr/wfile.26 -> pr/wfile.27) due to conflict with #12331.
  101. promag commented at 0:21 am on February 13, 2018: member

    Tested ACK 1fe5528. I have two cases I’d like to discuss:

    1. should -wallet=../foo or -wallet=foo/.. be possible?
    2. running bitcoind -regtest -wallet=. -wallet now gives:
    0************************
1EXCEPTION: St13runtime_error
2CDB: Can't open database wallet.dat (duplicates fileid 31e920000400000107a307f45b0d000000000000 from wallet.dat)
3bitcoin in AppInit()

    And before:

    0Error: Error loading wallet .. -wallet filename must be a regular file.
  102. ryanofsky force-pushed on Feb 14, 2018
  103. ryanofsky commented at 8:13 pm on February 14, 2018: member

    Rebased 1fe5528f4dea8b530022b9bf359426f2079848af -> 230f696ac105f41757c7e7625053941d483df0be (pr/wfile.27 -> pr/wfile.28) due to conflict with #12225. Added 1 commits 230f696ac105f41757c7e7625053941d483df0be -> c8c87343fcc7d86e9bec100a609a521c6c1e2d34 (pr/wfile.28 -> pr/wfile.29, compare) Squashed c8c87343fcc7d86e9bec100a609a521c6c1e2d34 -> 366c886fd7e1df95b2cdccde666eb53865e6c0c3 (pr/wfile.29 -> pr/wfile.30)

    I have two cases I’d like to discuss:

    Could point out what you would like these test cases to do? They seem strange.

    should -wallet=../foo or -wallet=foo/.. be possible?

    I don’t see either a use-case here, or a reason to worry about the relative directory syntax, but if there is a concern you can articulate, adding more restrictions on path strings would be possible. You may want to look at 8c014825a6dba47f5a262da05e26394ef95b60df and surrounding discussion above.

    bitcoind -regtest -wallet=. -wallet

    This is a weird invocation, and I’m not sure what you would want to happen with it. But the reason it fails to start with the “duplicates fileid” message is that this tries to load the default wallet twice, and any time you try to load two copies of the same wallet, you will see this.

  104. ryanofsky referenced this in commit c8c87343fc on Feb 14, 2018
  105. ryanofsky force-pushed on Feb 14, 2018
  106. promag commented at 9:15 pm on February 14, 2018: member
    I mean, either the path is absolute or it’s a name (which will be relative to walletdir). So anything relative (e.g. ., .., ../foo, foo/.., …) should fail. Unless there is a use case for relative paths?
  107. ryanofsky force-pushed on Mar 2, 2018
  108. Allow wallet files in multiple directories 
    Remove requirement that two wallet files can only be opened at the same time if
they are contained in the same directory.

This change mostly consists of updates to function signatures (updating
functions to take fs::path arguments, instead of combinations of strings,
fs::path, and CDBEnv / CWalletDBWrapper arguments).
    d8a99f65e5
  109. Allow wallet files not in -walletdir directory 
    Remove restriction that -wallet filenames can only refer to files in the
-walletdir directory.
    26c06f24e5
  110. Create new wallet databases as directories rather than files 
    This change should make it easier for users to make complete backups of wallets
because they can now just back up the specified `-wallet=<path>` path directly,
instead of having to back up the specified path as well as the transaction log
directory (for incompletely flushed wallets).

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.
    be8ab7d082
  111. ryanofsky force-pushed on Mar 3, 2018
  112. ryanofsky commented at 5:33 pm on March 6, 2018: member

    I mean, either the path is absolute or it’s a name (which will be relative to walletdir). So anything relative (e.g. ., .., ../foo, foo/.., …) should fail. Unless there is a use case for relative paths?

    The use cases for relative paths are pretty straightforward. Maybe you want to put your watch only wallets in a different directory than your wallets with private keys. Maybe you want to have different directories for different customers, or use nested directory structure, or use symlinks to avoid hardcoding paths that may change.

    But I’m not sure if you are concerned about relative paths, or if you are concerned about particular types of path syntax like .. or ., or something else. (It might help to state the concern explicitly.) If the concern is actually allowing .. or . path syntax, I guess it would be possible to add operating-system specific code to forbid it, although I don’t see what the benefit would be. It seems like a good approach to take with path strings in application code is to not manipulate them directly, but just pass them through to the OS and boost path libraries so they get interpreted in a consistent way.

    Rebased 366c886fd7e1df95b2cdccde666eb53865e6c0c3 -> 0f090a85233dc19a059b5a69a886554f543d6bdd (pr/wfile.30 -> pr/wfile.31) Rebased 0f090a85233dc19a059b5a69a886554f543d6bdd -> be8ab7d082228d09ca529d1a08730d7d5aacb0ed (pr/wfile.31 -> pr/wfile.32)

  113. in src/wallet/db.h:105 in d8a99f65e5 outdated 
    100@@ -100,9 +101,33 @@ class CWalletDBWrapper
101     }
102 
103     /** Create DB handle to real database */
104-    CWalletDBWrapper(CDBEnv *env_in, const std::string &strFile_in) :
105-        nUpdateCounter(0), nLastSeen(0), nLastFlushed(0), nLastWalletUpdate(0), env(env_in), strFile(strFile_in)
106+    CWalletDBWrapper(const fs::path& wallet_path, bool mock = false) :
107+        nUpdateCounter(0), nLastSeen(0), nLastFlushed(0), nLastWalletUpdate(0)
    jimpo commented at 2:33 pm on March 7, 2018:
    These initializers could be replaced with : CWalletDBWrapper() I believe.
  114. in src/wallet/wallet.cpp:3933 in d8a99f65e5 outdated 
    3929@@ -3929,8 +3930,7 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
3930 
3931     int64_t nStart = GetTimeMillis();
3932     bool fFirstRun = true;
3933-    std::unique_ptr<CWalletDBWrapper> dbw(new CWalletDBWrapper(&bitdb, walletFile));
3934-    CWallet *walletInstance = new CWallet(std::move(dbw));
3935+    CWallet *walletInstance = new CWallet(name, CWalletDBWrapper::Create(path));
    jimpo commented at 3:45 pm on March 7, 2018:
    It looks like this would leak in the case of any of the early error exists. Perhaps it’s better to make a unique_ptr, then release() on the last line of the method to prevent this?
  115. laanwj commented at 4:11 pm on March 7, 2018: member
    utACK be8ab7d082228d09ca529d1a08730d7d5aacb0ed
  116. laanwj removed this from the "Blockers" column in a project
  117. laanwj merged this on Mar 7, 2018
  118. laanwj closed this on Mar 7, 2018
  119. fanquake referenced this in commit 98bc27fb59 on Mar 7, 2018
  120. jimpo commented at 4:54 pm on March 7, 2018: contributor
    Oops, post merge review.
  121. in src/wallet/init.cpp:245 in be8ab7d082 
    251+        fs::file_type path_type = fs::symlink_status(wallet_path).type();
252+        if (!(path_type == fs::file_not_found || path_type == fs::directory_file ||
253+              (path_type == fs::symlink_file && fs::is_directory(wallet_path)) ||
254+              (path_type == fs::regular_file && fs::path(walletFile).filename() == walletFile))) {
255+            return InitError(strprintf(
256+                _("Invalid -wallet path '%s'. -wallet path should point to a directory where wallet.dat and "
    MarcoFalke commented at 3:12 pm on March 8, 2018:
    Very technical error message. Should not be translated, imo.
  122. virtload referenced this in commit 9e173c5cbf on Apr 4, 2018
  123. virtload referenced this in commit 52d07c54b0 on Apr 4, 2018
  124. laanwj referenced this in commit 0700b6f778 on Apr 9, 2018
  125. laanwj referenced this in commit 3fd0c2336a on May 29, 2018
  126. NickyYangYang commented at 9:47 am on July 16, 2018: none
    hello, The wallet.dat size is so large. I know there is so much keypairs in the file. But , what is the structure of the file? Is the same structure with the class CCryptoKeyStore?
  127. schancel referenced this in commit 797636646c on Apr 19, 2019
  128. jasonbcox referenced this in commit ce33ec89a9 on Oct 25, 2019
  129. PastaPastaPasta referenced this in commit c68dc2bf5e on Jan 31, 2020
  130. PastaPastaPasta referenced this in commit e2edefa22e on Feb 4, 2020
  131. PastaPastaPasta referenced this in commit 2c7b29bac5 on Feb 9, 2020
  132. UdjinM6 referenced this in commit ce2f97c28d on Feb 29, 2020
  133. PastaPastaPasta referenced this in commit cc2cd7291b on Mar 4, 2020
  134. PastaPastaPasta referenced this in commit 5c27c14f91 on Mar 23, 2020
  135. PastaPastaPasta referenced this in commit 3fbe3612b8 on Mar 28, 2020
  136. PastaPastaPasta referenced this in commit 9a6a558709 on Mar 29, 2020
  137. PastaPastaPasta referenced this in commit 58209be717 on Mar 31, 2020
  138. UdjinM6 referenced this in commit cec34a25d6 on Mar 31, 2020
  139. PastaPastaPasta referenced this in commit 9bedb900a9 on Apr 1, 2020
  140. PastaPastaPasta referenced this in commit d53070441d on Apr 12, 2020
  141. PastaPastaPasta referenced this in commit d44d3505fb on Apr 14, 2020
  142. PastaPastaPasta referenced this in commit fb1778288f on Apr 14, 2020
  143. PastaPastaPasta referenced this in commit 9a532bf469 on Apr 14, 2020
  144. PastaPastaPasta referenced this in commit bd074f9622 on Apr 15, 2020
  145. PastaPastaPasta referenced this in commit 0572def134 on Apr 15, 2020
  146. PastaPastaPasta referenced this in commit bc0ae9e16c on Apr 15, 2020
  147. PastaPastaPasta referenced this in commit 632ed10ddf on Apr 15, 2020
  148. PastaPastaPasta referenced this in commit 617c7ba9a5 on Apr 15, 2020
  149. PastaPastaPasta referenced this in commit 215a24cc43 on Apr 16, 2020
  150. PastaPastaPasta referenced this in commit 0165e264e5 on Apr 16, 2020
  151. PastaPastaPasta referenced this in commit 92704d4cb6 on Apr 17, 2020
  152. PastaPastaPasta referenced this in commit 284373b14f on Apr 18, 2020
  153. PastaPastaPasta referenced this in commit bb39a539c3 on Apr 18, 2020
  154. UdjinM6 referenced this in commit 44bbd48863 on Apr 23, 2020
  155. PastaPastaPasta referenced this in commit 0c9b1f5ac9 on Apr 23, 2020
  156. PastaPastaPasta referenced this in commit 7999080c04 on Apr 23, 2020
  157. meshcollider referenced this in commit 8ed37f6c84 on Oct 15, 2020
  158. sidhujag referenced this in commit c7e926b4cd on Oct 16, 2020
  159. PastaPastaPasta referenced this in commit 160061dc59 on Dec 16, 2020
  160. PastaPastaPasta referenced this in commit faa6361c63 on Dec 18, 2020
  161. ckti referenced this in commit d26cd3fea8 on Mar 28, 2021
  162. ckti referenced this in commit 8b12185e22 on Mar 28, 2021
  163. ckti referenced this in commit d1570a7efb on Mar 28, 2021
  164. ckti referenced this in commit 4329de6914 on Mar 28, 2021
  165. gades referenced this in commit 2ca7534241 on Jun 30, 2021
  166. gades referenced this in commit 75723e599e on Jun 30, 2021
  167. gades referenced this in commit 4c10e86e9b on Jun 30, 2021
  168. furszy referenced this in commit b04e1f5a90 on Jul 23, 2021
  169. malbit referenced this in commit d0080d6d1c on Nov 5, 2021
  170. malbit referenced this in commit 53979b2407 on Nov 5, 2021
  171. malbit referenced this in commit 0f6a33b5c1 on Nov 5, 2021
  172. malbit referenced this in commit 6329458696 on Nov 5, 2021
  173. gades referenced this in commit 5c0603a06b on Feb 12, 2022
  174. DrahtBot locked this on Feb 15, 2022


ryanofsky luke-jr gmaxwell meshcollider laanwj promag sipa TheBlueMatt PierreRochard jnewbery alphaaurigae Sjors jimpo MarcoFalke NickyYangYang

Labels
Wallet

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-22 03:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me