Cleanups + nit fixes for walletdir PR #11726
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
laanwj
reviewed
Nov 19, 2017
src/wallet/walletutil.cpp
Outdated
Member
There was a problem hiding this comment.
This is a tricky case but after thinking a bit, I think this should be unconditional. If the user wants to use their walletdir with testnet/regtest, they should create a subdir for it in the walletdir.
Otherwise this will still result in wallet directory collision by default if -walletdir is specified in bitcoin.conf.
We really want to avoid the case where accidentally, say, a regtest wallet is used on mainnet or vice versa.
Member
There was a problem hiding this comment.
After discussion on IRC, I realized maybe we don't want to do this at all and just wait for #10996 to go in. This will remove the issue by making it possible to set a separate walletdir per network.
Contributor
Author
There was a problem hiding this comment.
Agreed, #10996 or something similar would be much cleaner. Removed.
promag
reviewed
Nov 19, 2017
src/wallet/init.cpp
Outdated
Contributor
There was a problem hiding this comment.
Remove default value "" here and below?GetArg()must be called with default value.
src/wallet/init.cpp
Outdated
Contributor
There was a problem hiding this comment.
Suggestion:
if (exists) {
return "not a directory";
}
return "does not exists";
Member
There was a problem hiding this comment.
nit: (in case you touch the pull for other reasons): Add space after if (or use clang-format).
promag
reviewed
Nov 27, 2017
doc/files.md
Outdated
Contributor
There was a problem hiding this comment.
Follow blktree/ and blocks/index/* example:
* db.log: wallet database log file; replaced by wallets/ directory in 0.16.0
* wallets/db.log; since 0.16.0
Same for wallet.dat.
Contributor
Author
|
@promag I've made an attempt at updating the files.md as you suggested, feel free to suggest a different change though if you're not happy still :) |
TheBlueMatt
reviewed
Dec 8, 2017
doc/files.md
Outdated
Contributor
There was a problem hiding this comment.
since 0.8.0; since 0.16.0...wait...huh
promag
reviewed
Dec 10, 2017
Contributor
promag
left a comment
promag
left a comment
There was a problem hiding this comment.
Add tests like
bitcoin/test/functional/feature_logging.py
Lines 33 to 34 in 59d3dc8
src/wallet/init.cpp
Outdated
Contributor
There was a problem hiding this comment.
IMO this is better Specified -walletdir \"%s\" is not a directory (no period and references parameter name).
src/wallet/init.cpp
Outdated
Contributor
Author
|
Updated as per @promag comments, tests are already present in multiwallet.py but I've updated them with the new error messages |
ryanofsky
reviewed
Dec 18, 2017
Member
|
Looks sufficiently reviewed. Going to merge. |
maflcko
pushed a commit
that referenced
this pull request
Dec 20, 2017
aac6b3f Update files.md for new wallets/ subdirectory (MeshCollider) b673429 Cleanups for walletdir PR (MeshCollider) Pull request description: This addresses the remaining nits from #11466 - Updates `doc/files.md` with respect to the new default wallet directory - Fixes @promag and @laanwj's error message nit, and Jonas' release notes nit - ~Addresses @laanwj's net-specific wallet subdirectory concern in the case that a walletdir is specified~ - Changes the #includes from "" to <> style after #11651 Tree-SHA512: b86bf5fdc4de54c1b0f65b60a83af3cf82b35d216ce9c0de724803bfba6934796238b6c412659dcc29ae2e3e856d4eb97ae777c80f36f4089d8acecfddefe9aa
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Mar 4, 2020
aac6b3f Update files.md for new wallets/ subdirectory (MeshCollider) b673429 Cleanups for walletdir PR (MeshCollider) Pull request description: This addresses the remaining nits from bitcoin#11466 - Updates `doc/files.md` with respect to the new default wallet directory - Fixes @promag and @laanwj's error message nit, and Jonas' release notes nit - ~Addresses @laanwj's net-specific wallet subdirectory concern in the case that a walletdir is specified~ - Changes the #includes from "" to <> style after bitcoin#11651 Tree-SHA512: b86bf5fdc4de54c1b0f65b60a83af3cf82b35d216ce9c0de724803bfba6934796238b6c412659dcc29ae2e3e856d4eb97ae777c80f36f4089d8acecfddefe9aa
gades
pushed a commit
to cosanta/cosanta-core
that referenced
this pull request
Jun 30, 2021
aac6b3f Update files.md for new wallets/ subdirectory (MeshCollider) b673429 Cleanups for walletdir PR (MeshCollider) Pull request description: This addresses the remaining nits from bitcoin#11466 - Updates `doc/files.md` with respect to the new default wallet directory - Fixes @promag and @laanwj's error message nit, and Jonas' release notes nit - ~Addresses @laanwj's net-specific wallet subdirectory concern in the case that a walletdir is specified~ - Changes the #includes from "" to <> style after bitcoin#11651 Tree-SHA512: b86bf5fdc4de54c1b0f65b60a83af3cf82b35d216ce9c0de724803bfba6934796238b6c412659dcc29ae2e3e856d4eb97ae777c80f36f4089d8acecfddefe9aa
furszy
added a commit
to PIVX-Project/PIVX
that referenced
this pull request
Jul 23, 2021
…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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This addresses the remaining nits from #11466
doc/files.mdwith respect to the new default wallet directoryAddresses @laanwj's net-specific wallet subdirectory concern in the case that a walletdir is specified