-
Notifications
You must be signed in to change notification settings - Fork 720
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Wallet] Clean wallet auto-backup "feature" #2369
Conversation
2b35b91
to
21f8363
Compare
7e612ee
to
2b22c90
Compare
Now that #2362 was merged, this is ready to go. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK.
Although, as the tests show, this has a bug. See comments inlined.
Aside from that, it would be good to rebase this one on top of #2351 (which is already based on #2337) to minimize conflicts.
Also, basing this work on top of #2351, you could do another step in the refactoring.
At the moment we have an ordering issue: on the first run, InitAutoBackupWallet
is called before creating the actual wallet (which is done in step 8, InitLoadWallet
), so it always logs a warning and it never backups the file.
Would be good to create the new wallet, and then back it up.
In #2351 we move the backup step inside InitLoadWallet
, so, with a little more refactoring, we could backup the actual running/loaded wallets, calling directly BackupWallet
(merging it with AutoBackupWallet
, essentially flushing the database before copying the file).
2b22c90
to
f3e4a0c
Compare
54b80f3
to
7a3e9e9
Compare
Added a bit more work here.
|
7a3e9e9
to
82bfe4d
Compare
Rebased on master, fixing conflicts. Ready to go. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking nice, but found a couple more issues (see comments inlined).
Also, one minor idea over d0eda25.
Currently, inside InitLoadWallet
, we are doing this :
- try to backup. if the wallet file is not found, don't warn.
- try to load the wallet. if it doesn't exist, create a new one, and set
fFirstRun
to true. - if
fFirstRun
is true, then try the backup once again.
Wouldn't it be cleaner to just load/create the wallet before trying the backup, and keep the warning for missing source file (random-zebra@bf90512)?
We spoke about this few times already, the backup at startup was added in case of having a problem at the initialization & load phase and for some reason the wallet file gets corrupted, so the file gets copied before loading it. Different software versions could try to open the wallet db in different manners, even modify it (we should try to avoid anything like this..), so having a simple file copy backup before doing any operation over the file is by far simpler than parse and load it and then re-serialize everything to save it to disk again.
Not sure if you are thinking that it will do the backup twice with the last "once again". In case you are thinking it, it will not happen. |
Plus fix "Failed to create backup..." error at startup when the wallet hasn't been created yet.
It will attempt to do it twice. First time it will fail of course. That's why you had to remove the warning. |
You know something, thinking this further, aside from the refactoring and code cleanup that made here, the problem that have with this flow is the following one: if the wallet file is already corrupted and the user tries to open the wallet +10 times, then every backup file will be a copy of the corrupted file, the rolling backup process will remove the good backups and create only corrupted ones. Will move it to the backup after the unserialization and wallet load, it could introduce corruptions only if we do nasty nasty things on the init/load process such as a data write etc (which will not happen while we are around..) |
82bfe4d
to
f36c4d0
Compare
Updated now, cherry-picked your commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK f36c4d0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK ce4ba966f25d7c82c5e2484a86ee5efc102704f2
…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
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
Was checking #2362 and wasn't able to contain myself from cleaning redundancies and code deduplication on the wallet auto-backup "feature".