[Wallet] Introduce wallets directory configuration and external wallet files capabilities #2423

furszy · 2021-06-15T23:34:49Z

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:

Refactor: Modernize disallowed copy constructors/assignment bitcoin/bitcoin#11351 -> Refactor: Modernize disallowed copy constructors/assignment.
Specify custom wallet directory with -walletdir param bitcoin/bitcoin#11466 -> Specify custom wallet directory with -walletdir param.
Cleanups + nit fixes for walletdir PR bitcoin/bitcoin#11726 -> Cleanups + nit fixes for -walletdir work.
Add a lock to the wallet directory bitcoin/bitcoin#11904 -> Add lock to the wallet directory.
External wallet files bitcoin/bitcoin#11687 -> External wallet files support.
[docs] Clarify -walletdir usage bitcoin/bitcoin#12166 -> Doc better -walletdir description.
Error if relative -walletdir is specified bitcoin/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.

furszy · 2021-07-06T15:13:25Z

rebased on master after #2369 merge.

random-zebra

Overall looking good.
Aside from some comments/nits inlined, though, wallet backup got broken (both automatic and on-demand) as the source path doesn't take into account the specific wallet's directory (see bitcoin#13667).
Can pick the two commits in https://github.com/random-zebra/PIVX/tree/pr_2423, if you want, which fix the backups and add some more cleanup.

src/init.cpp

src/pivxd.cpp

src/wallet/walletdb.cpp

test/functional/wallet_multiwallet.py

src/init.cpp

src/wallet/walletdb.cpp

src/wallet/db.h

src/wallet/wallet.cpp

doc/release-notes.md

random-zebra · 2021-07-20T11:19:05Z

doc/release-notes.md

+Newly created wallet format
+---------------------------


commit f6690b2

Same as the previous commit, these notes could be slightly reorganized, as -wallet/-walletdir options are both included in this release for the first time.
But we can work on this on a separate PR too.

yeah agree, while we don't forget it, it can be done here or in a follow up PR.

furszy · 2021-07-20T22:25:02Z

Nice review and follow up investigation zebra 👍 ☕ . Will tackle the comments and check your commits.

random-zebra

Few more notes.
As the full path to the wallet file is now an implementation detail, it is no longer available outside the wallet class.
If we really need to display it, we could introduce something like:

fs::path CWalletDBWrapper::GetPathToFile() { return env->Directory() / strFile; }
fs::path CWallet::GetPathToDBFile() { return dbw->GetPathToFile(); }

but I think that displaying just the wallet location (dir and/or file, depending on -wallet flags), already available with GetWalletDir()+GetName() is enough.

src/qt/walletmodel.cpp

src/qt/pivx/settings/settingsinformationwidget.cpp

src/qt/rpcconsole.cpp

src/wallet/db.cpp

Use C++11's better capability of expressing an interface of a non-copyable class by publicly deleting its copy ctor and assignment operator instead of just declaring them private.

…th calling LockDirectory multiple times on the same directory, or from multiple threads. It also fixes the build on OpenBSD.

…ry, introduced in btc#11281.

backupwallet was broken for multiwallets in their own directories (i.e. something like DATADIR/wallets/mywallet/wallet.dat). In this case, the backup would use DATADIR/wallets/wallet.dat as source file and not take the specific wallet's directory into account. This led to either an error during the backup (if the wrong source file was not present) or would silently back up the wrong wallet; especially the latter behaviour can be quite bad for users.

Co-authored-by: random-zebra <[email protected]>

random-zebra

One tiny point in the last commit. Otherwise tested ACK 533c8fc383c31b379c879c7776badadcb2d57fa2

src/qt/pivx/settings/settingsinformationwidget.cpp

furszy · 2021-07-22T13:13:14Z

Updated per feedback.

random-zebra

utACK 056f4e8

Fuzzbawls

ACK 056f4e8

7a5d181 Use the character based overload for std::string::find. (Alin Rus) c19401b Don't use pass by reference to const for cheaply-copied types (bool, char, etc.). (practicalswift) 4c5fe36 [Refactor] Remove unused fQuit var from checkqueue.h (donaloconnor) fda7a5f Cleanup: remove unneeded header includes (random-zebra) ac2476c Add test cases covering the relevant key length boundaries: 64 bytes +/- 1 byte for HMAC-SHA256 and 128 bytes +/- 1 byte for HMAC-SHA512 (practicalswift) a346262 Fix code constness in CBlockIndex::GetAncestor() overloads (Dan Raviv) 65e3f4e Refactor: More range-based for loops (random-zebra) dd3d3c4 Use range-based for loops (C++11) when looping over map elements (practicalswift) 5a7750a Move RPC registration out of AppInitParameterInteraction (Russell Yanofsky) 402b4c4 Use compile-time constants instead of unnamed enumerations (practicalswift) bbac2d0 [Trivial] Fix indentation in coins.cpp (random-zebra) e539c62 Small refactor of CCoinsViewCache::BatchWrite() (Dan Raviv) ec91759 Use MakeUnique<T>(...) instead of std::unique_ptr<T>(new T(...)) (random-zebra) 93487b1 Use unique_ptr for pcoinscatcher/pcoinsdbview/pcoinsTip/pblocktree (random-zebra) ff43d69 Use unique_ptr for pdbCopy (Db) and fix potential memory leak (practicalswift) b4d9641 Use unique_ptr for dbenv (DbEnv) (practicalswift) 36108b9 Use unique_ptr for pfilter (CBloomFilter) (practicalswift) ff1c454 Use unique_ptr for sem{Addnode,Outbound} (CSemaphore) (practicalswift) 93daf17 Use unique_ptr for httpRPCTimerInterface (HTTPRPCTimerInterface) (practicalswift) 020ac16 Init: Remove redundant exit(EXIT_FAILURE) instances and replace with (random-zebra) b9f5d1f Remove duplicate uriParts.size() > 0 check (practicalswift) 440d961 Remove redundant check (!ecc is always true) (practicalswift) bfd295b Remove redundant NULL checks after new (practicalswift) 97aad32 Make fUseCrypto atomic (MeshCollider) 2711f78 Consistent parameter names in txdb.h (MeshCollider) d40df3a Fix race for mapBlockIndex in AppInitMain (random-zebra) 03b7766 Cleanup: remove unused functions to Hash the concat of 4 or more objects (random-zebra) c520e0f Remove some unused functions and methods (Pieter Wuille) 508f1a1 range-based loops and const qualifications in net.cpp (Marko Bencun) 79b1e50 Refactor: implement CPubKey::data() (random-zebra) 614d26c Refactor: more &vec[0] to vec.data() (random-zebra) 02b6337 Changing &vec[0] to vec.data(), what 9804 missed (MeshCollider) c1c8b05 Ensure that data types are consistent (jjz) 732bb9d Fix potential null dereferences (MeshCollider) 80f35f9 Remove unreachable code (practicalswift) Pull request description: This is a collection of simple refactorings coming from upstream Bitcoin v0.16 (adapting/extending to PIVX-specific code where needed). Pull requests backported: - bitcoin#10845 (practicalswift) - bitcoin#11238 (MeshCollider) - bitcoin#11232 (jjz) - bitcoin#10793 (MeshCollider) - bitcoin#10888 (benma) - ~~bitcoin#11351 (danra)~~ [edit: removed - included in #2423] - bitcoin#11385 (sipa) - bitcoin#11107 (MeshCollider) - bitcoin#10898 (practicalswift) - bitcoin#11511 (donaloconnor) - bitcoin#11043 (practicalswift) - bitcoin#11353 (danra) - bitcoin#10749 (practicalswift) - bitcoin#11603 (ryanofsky) - bitcoin#10493 (practicalswift) - bitcoin#11337 (danra) - bitcoin#11516 (practicalswift) - bitcoin#10574 (practicalswift) - bitcoin#12108 (donaloconnor) - bitcoin#10839 (practicalswift) - bitcoin#12159 (kekimusmaximus) ACKs for top commit: Fuzzbawls: ACK 7a5d181 furszy: re-ACK 7a5d181 after rebase, no code changes. Merging.. Tree-SHA512: d92f5df47f443391a6531274a2efb9a4882c62d32eff628f795b3abce703f108c8b40ec80ac841cde6c5fdd5c9ff2b6056a31546ac2edda279f5f18fccc99c32

8756130 [BUG] Fix broken tutorial wizard (random-zebra) Pull request description: The GUI tutorial screen (where the user selects the language and gets the first few "beginner tips") is supposed to be fired when the wallet.dat file is not found, before creating it for the first time. In case of multiple wallets (using the `wallet` option in the config file, or as startup flag), the tutorial screen should not be fired if *at least* one wallet.dat already exists. The check to decide whether or not to prompt the tutorial screen was broken in #2423. The `-wallet` arguments can now represent paths to existing directories or symlinks (by default it is empty, representing the path relative to the datadir, i.e. the datadir itself). It does not include the *filename* anymore (unless explicitly provided to `-wallet`). Therefore the tutorial screen is never created on the first launch, as it should. Also, we are using `GetArg`, which returns the last argument from command line (or first argument in conf file), and not checking all the files with `GetArgs`. ACKs for top commit: furszy: nice catch 👌👌, ACK 8756130 Fuzzbawls: utACK 8756130 Tree-SHA512: ac3f67a8b975397e9136b9b00a88f5fb88d6eeceb613256d99aa2e9dc604df9d6e3e098dd06f5c9604a2ef6b3b255c88c5aa7b45e667f10a56f07402d3683009

63e0be6 [Remove] By-pass logprint-scanner restriction. (furszy) 280ced3 utils: Fix broken Windows filelock (Chun Kuan Lee) be89860 utils: Convert Windows args to utf-8 string (Chun Kuan Lee) e8cfa6e Call unicode API on Windows (Chun Kuan Lee) 1a02a8a tests: Add test case for std::ios_base::ate (Chun Kuan Lee) 2e57cd4 Move boost/std fstream to fsbridge (furszy) 9d8bcd4 utils: Add fsbridge fstream function wrapper (Chun Kuan Lee) d59d48d utils: Convert fs error messages from multibyte to utf-8 (ken2812221) 9ef58cc Logging: use "fmterr" variable name for errors instead of general "e" that can be used by any other function. (furszy) dd94241 utils: Use _wfopen and _wreopen on Windows (Chun Kuan Lee) 3993641 add unicode compatible file_lock for Windows (Chun Kuan Lee) 48349f8 Provide relevant error message if datadir is not writable. (murrayn) Pull request description: As the software is currently using the ANSI encoding on Windows, the user's language settings could affect the proper functioning of the node/wallet, to the point of not be able to open some non-ASCII name files and directories. This solves the Windows encoding issues, completing the entire bitcoin#13869 work path (and some other required backports). Enabling for example users that use non-english characters in directories and file names to be accepted. Backported PRs: * bitcoin#12422. * bitcoin#12630. * bitcoin#13862. * bitcoin#13866. * bitcoin#13877. * bitcoin#13878. * bitcoin#13883. * bitcoin#13884. * bitcoin#13886. * bitcoin#13888. * bitcoin#14192. * bitcoin#13734. * bitcoin#14426. This is built on top of other two PRs that i have open #2423 and #2369. Solves old issues #940 and #2163. TODO: * Backport `assert_start_raises_init_error` and `ErrorMatch` in TestNode` (bitcoin#12718) ACKs for top commit: Fuzzbawls: ACK 63e0be6 random-zebra: ACK 63e0be6 and merging... Tree-SHA512: cb1f7c23abb5b7b3af50bba18652cc2cad93fd7c2fca9c16ffd3fee34c4c152a3b666dfa87fe6b44c430064dcdee4367144dcb4a41203c91b0173b805bdb3d7d

57bb926 [Doc] More v5.3 release notes (random-zebra) f935cdd [Doc] Add v5.3.0 finalized release-notes (furszy) Pull request description: Finalization of the v5.3.0 release notes. Pending work: * Comment in #2423 (comment). (multi-wallet sections need to be reordered a bit). ACKs for top commit: random-zebra: ACK 57bb926 Fuzzbawls: ACK 57bb926 Tree-SHA512: 05d00650b7c4cf7b562550f341a72f1ea4dc8edef7669e83ab26202fabc7b0eed6a0a02f9bac6d93ed3d87efdb4fd3e63c3b7213d0d66f7ea1b5f56b65f0e7c1

furszy self-assigned this Jun 15, 2021

furszy added this to the 6.0.0 milestone Jun 16, 2021

furszy added Upstream Wallet labels Jun 16, 2021

furszy force-pushed the 2021_wallets_dir branch 4 times, most recently from 52d5073 to ea5a28f Compare June 22, 2021 19:49

furszy force-pushed the 2021_wallets_dir branch 6 times, most recently from 381621b to 39bd730 Compare June 24, 2021 03:29

furszy mentioned this pull request Jun 24, 2021

Solve filename and command line encoding issues on Windows #2449

Merged

random-zebra added the Refactoring label Jun 28, 2021

random-zebra mentioned this pull request Jul 2, 2021

[Refactoring] Bitcoin 0.16 cherries #2462

Merged

furszy force-pushed the 2021_wallets_dir branch from 39bd730 to 2ebf8dd Compare July 6, 2021 15:12

furszy force-pushed the 2021_wallets_dir branch 2 times, most recently from 513e702 to dad3bec Compare July 12, 2021 22:56

furszy force-pushed the 2021_wallets_dir branch from dad3bec to c04c238 Compare July 14, 2021 16:40

furszy mentioned this pull request Jul 18, 2021

[Tests] Clean and lint the python regtest suite #2452

Merged

furszy requested a review from Fuzzbawls July 18, 2021 23:05

furszy force-pushed the 2021_wallets_dir branch from c04c238 to 2679f4b Compare July 19, 2021 01:53

random-zebra reviewed Jul 20, 2021

View reviewed changes

random-zebra reviewed Jul 21, 2021

View reviewed changes

src/qt/walletmodel.cpp Outdated Show resolved Hide resolved

src/qt/pivx/settings/settingsinformationwidget.cpp Outdated Show resolved Hide resolved

src/qt/rpcconsole.cpp Show resolved Hide resolved

src/wallet/db.cpp Show resolved Hide resolved

furszy force-pushed the 2021_wallets_dir branch from 2679f4b to 9e21316 Compare July 21, 2021 14:48

Refactor: Modernize disallowed copy constructors/assignment

a1bef4f

Use C++11's better capability of expressing an interface of a non-copyable class by publicly deleting its copy ctor and assignment operator instead of just declaring them private.

furszy force-pushed the 2021_wallets_dir branch from 89d82b0 to b1bdd08 Compare July 21, 2021 16:30

furszy and others added 11 commits July 21, 2021 16:38

Test: Use specific testing setups for wallet_zkeys_tests tests

9ae619a

util: Fix multiple use of LockDirectory This commit fixes problems wi…

16b4651

…th calling LockDirectory multiple times on the same directory, or from multiple threads. It also fixes the build on OpenBSD.

test: Add unit test for LockDirectory Add a unit test for LockDirecto…

4cae8dc

…ry, introduced in btc#11281.

db: fix db path not removed from the open db environments map.

565abcd

wallet_tests: mock wallet db.

351d2c8

[BUG] Sanitize wallet name in GetUniqueWalletBackupName

12a1e39

[Refactor][Bug] Fix automatic backups, final code deduplication

91b112b

[Refactor] First load all wallets, then back em

e7aa6bd

Implement and connect CWallet::GetPathToFile to GUI.

524103f

Co-authored-by: random-zebra <[email protected]>

bugfix: Remove dangling wallet env instance

f765611

furszy force-pushed the 2021_wallets_dir branch from b1bdd08 to 533c8fc Compare July 21, 2021 19:38

random-zebra previously approved these changes Jul 22, 2021

View reviewed changes

src/qt/pivx/settings/settingsinformationwidget.cpp Outdated Show resolved Hide resolved

random-zebra added the Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes label Jul 22, 2021

[GUI] settings information widget setting the correct data directory.

056f4e8

furszy dismissed random-zebra’s stale review via 056f4e8 July 22, 2021 13:11

furszy force-pushed the 2021_wallets_dir branch from 533c8fc to 056f4e8 Compare July 22, 2021 13:11

random-zebra approved these changes Jul 22, 2021

View reviewed changes

Fuzzbawls approved these changes Jul 23, 2021

View reviewed changes

furszy merged commit b04e1f5 into PIVX-Project:master Jul 23, 2021

random-zebra mentioned this pull request Jul 23, 2021

[BUG][GUI] Fix broken tutorial screen #2483

Merged

furszy mentioned this pull request Aug 23, 2021

[Doc] Finalize v5.3.0 Release Notes #2526

Merged

random-zebra removed the Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes label Aug 24, 2021

furszy deleted the 2021_wallets_dir branch June 23, 2023 21:04

[Wallet] Introduce wallets directory configuration and external wallet files capabilities #2423

[Wallet] Introduce wallets directory configuration and external wallet files capabilities #2423

Uh oh!

Conversation

furszy commented Jun 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

furszy commented Jul 6, 2021

Uh oh!

random-zebra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

random-zebra Jul 20, 2021

Choose a reason for hiding this comment

Uh oh!

furszy Jul 20, 2021

Choose a reason for hiding this comment

Uh oh!

furszy commented Jul 20, 2021

Uh oh!

random-zebra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

random-zebra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

furszy commented Jul 22, 2021

Uh oh!

random-zebra left a comment

Choose a reason for hiding this comment

Uh oh!

Fuzzbawls left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

furszy commented Jun 15, 2021 •

edited

Loading