This could be the basic for later adding backup possibilities for Bitcoin-Qt including visual elements for latest and amount of backups as also validity-check-display of backup files.
Would it be possible to use the dumpwallet RPC output instead, it's much easier to parse and likely to remain usable in more software longer. On the downside, it's not natively encrypted.
@sipa hmm... it would be more open to other wallets. correct. On the other hand; how easy would it be for endusers to restore a backup? I think there is no easy way to import a RPC dumped wallet. IMO, endusers should just have <n> valid file(s) where they can restore. But a reasonable question is, how users could load a bitcoin-core wallet into other wallet-apps. But this might be out of scope for the backup-process because it affects also the non-backup wallet.dat.
Yes there is a way to import it, using importwallet.
@sipa: ah. right. Somebody should update https://en.bitcoin.it/wiki/Original_Bitcoin_client/API_calls_list :) But the enduser then could not just restore the wallet.dat file... or we could detect if the wallet.dat is RPC dumped file and import it straight. I would prefer to have a restore possibility on file level.
@jonasschnelli the GUI has no way to restore a backup at all - whether it's done using backupwallet or dumpwallet. That definitely needs fixing, but I would really advise against using the BDB-based format for anything but legacy.
@sipa i just thought through the whole thing again. Your right. Restore per file won't work anyway without rescan everything. I'll change it to use RPC walletdumped format. Enduser with limited IT understanding should import it through the Qt client (where i will focus on as soon as the basic things are sorted out).
@sipa i only see a huge drawback when using the walletdumps-txt: encryption. Unencrypted wallet dumps is IMO very risky. Of course we could encrypt the dumps before the go to the filesystem. But this lead to the question what passphrase/key we use for encrypting the backups dumps. Maybe adding a "backup-key" to the wallet would be a overhead? Any ideas how we could solve that in a nice way?
Good point about dumpwallet format not being encrypted. We should definitely support that.
For example Schildback's Bitcoin Wallet uses a format similar to our dumpwallet format, one line per key, but encrypts the file before writing. This can be decrypted from the command line using openssl enc -d -aes-256-cbc -a -in (see https://github.com/schildbach/bitcoin-wallet/blob/master/wallet/README.recover ), or of course imported back into the application.
As for the key, would be multiple options there:
I think we should encrypt the dumps with a key derived from the same password the wallet is encrypted.
Additional to that, for experience users, we could allow to set a base64 encoded EC PubKey in the bitcoinconf. The dumps then could be encrypted aes-256-cbc creating a random key with EVP_Seal. The encrypted key length and data will be stored in the dump. With that, experience users could do encrypted backups with keeping their backup key secret.
Please don't introduce yet another encryption format in the system if it can all be avoided, analyizing and maintaining two is additional work I'd rather avoid. The simpler thing is to just backup the existing files, then we're sure to get all the data, sure that it's already encrypted and not creating additional exposure, etc.
@gmaxwell agreed. Than i keep the pull request as it is. For now we backup just the wallet.dat. Dumping is IMO not a backup because it lacks of metadata (dump != backup). A backup should be somehow a 1:1 copy.
If a user enables -backupsdumps AND -backupsallowunencrypted he can do unencrypted dumps as backups.
Before working on encrypted dumps as backup possibilities i would prefer to implement the/a backup mechanism in the GUI.
2301 | + } 2302 | + mapKeyBirth.clear(); 2303 | + std::sort(vKeyBirth.begin(), vKeyBirth.end()); 2304 | + 2305 | + // produce output 2306 | + stream << strprintf("# Wallet dump created by Bitcoin %s (%s)\n", CLIENT_BUILD, CLIENT_DATE);
By "Bitcoin Core" :)?
17 | + 18 | #include <boost/filesystem.hpp> 19 | #include <boost/foreach.hpp> 20 | #include <boost/scoped_ptr.hpp> 21 | #include <boost/thread.hpp> 22 | +#include <boost/algorithm/string.hpp>
You were missing a few months here, so this will be my only nit for you ;). Can you please try to use alphabetical ordering for such includes (see e.g. most other files). This belongs to the top of the boost includes.
892 | @@ -889,6 +893,157 @@ bool BackupWallet(const CWallet& wallet, const string& strDest) 893 | return false; 894 | } 895 | 896 | +std::map<std::time_t, filesystem::path> CWalletDB::GetAvailableBackups() 897 | +{ 898 | + filesystem::path backupDir; 899 | + std::string strBackupsDir = GetArg("-backupspath", "");
IMHO the default path could be set as the second parameter here and directly converted to a boost::path perhaps? That could remove the if-else below.
hmm.. the GetDataDir() (which must be part of the default backup path) returns a boost::filesystem::path instance which is not compatible with GetArg's default argument. Converting the GetDataDir into a std::string first would probably end up in the same "mess".
Or do you see a other solution?
911 | + return backupFiles; 912 | + 913 | + filesystem::directory_iterator it(backupDir), eod; 914 | + BOOST_FOREACH(filesystem::path const &p, std::make_pair(it, eod)) 915 | + { 916 | + if(is_regular_file(p) && (ends_with(p.string(), BACKUP_BDB_EXTENSION) || ends_with(p.string(), BACKUP_DUMP_EXTENSION)))
Nit: Some comments in the code would be nice.
917 | + backupFiles.insert(std::pair<std::time_t, filesystem::path>(filesystem::last_write_time( p ), p)); 918 | + } 919 | + } 920 | + catch(const filesystem::filesystem_error &e) { 921 | + LogPrintf("error retrieving available backups (%s)\n", e.what()); 922 | + return backupFiles;
Is this return needed or will we reach the function end after the catch anyway?
I would say it's needed. When calling GetAvailableBackups you then can expect a empty hash-map even when there was a error.
@Diapolo okay, did overhaul the code, added some comments, etc. You might check it again.
372 | + 373 | +#ifdef ENABLE_WALLET 374 | + strUsage += "\n" + _("Backup options:") + "\n"; 375 | + strUsage += " -backups " + strprintf(_("Enable auto creation of UNENCRYPTED backups (dumps) (default: %u)"), DEFAULT_BACKUPS_ENABLED) + "\n"; 376 | + strUsage += " -backupsallowunencrypted " + strprintf(_("Allow backups of unencrypted wallets (default: %u)"), DEFAULT_ALLOW_UNENCRYPTED_BACKUPS) + "\n"; 377 | + strUsage += " -backupspath=<dir> " + strprintf(_("Specify absolut path to backups directory (default: %s)"), strprintf("<datadir>/%s", DEFAULT_BACKUPS_DIR)) + "\n";
"absolute"
367 | @@ -368,7 +368,16 @@ std::string HelpMessage(HelpMessageMode mode) 368 | strUsage += " -rpcsslcertificatechainfile=<file.cert> " + strprintf(_("Server certificate file (default: %s)"), "server.cert") + "\n"; 369 | strUsage += " -rpcsslprivatekeyfile=<file.pem> " + strprintf(_("Server private key (default: %s)"), "server.pem") + "\n"; 370 | strUsage += " -rpcsslciphers=<ciphers> " + strprintf(_("Acceptable ciphers (default: %s)"), "TLSv1.2+HIGH:TLSv1+HIGH:!SSLv2:!aNULL:!eNULL:!3DES:@STRENGTH") + "\n"; 371 | - 372 | + 373 | +#ifdef ENABLE_WALLET 374 | + strUsage += "\n" + _("Backup options:") + "\n"; 375 | + strUsage += " -backups " + strprintf(_("Enable auto creation of UNENCRYPTED backups (dumps) (default: %u)"), DEFAULT_BACKUPS_ENABLED) + "\n"; 376 | + strUsage += " -backupsallowunencrypted " + strprintf(_("Allow backups of unencrypted wallets (default: %u)"), DEFAULT_ALLOW_UNENCRYPTED_BACKUPS) + "\n";
So I need to specify two options to enable backups (assuming an unencrypted wallet)? Why?
I've started it very restrained. Maybe once the -backup feature could be enabled by default.
I think enabling two options for making unencrypted backups is okay because it's somehow bad practice.
Any NACKs ACKs? I would like to extend this to the GUI.
@jonasschnelli Concept ACK, haven't looked at the code. I promise I will, but right now I cannot keep up with the number of pulls anymore.
rebased
- allow to do automatic backups (wallet.dat copy or dumps) when keypool gets refilled
- by default backups are turned off
- by default only encrypted wallets are allowed for backup
- user can choose to backup dumps instead of the wallet.dat file
rebased.
I'm no longer convinced that this PRs solution is going into the right direction. As example: what means a encrypted wallet? Why are metadata like labels and comments (which could harm privacy) not encrypted in a encrypted wallet? How would this perform after there is support for Bip32?
@jonasschnelli Things other than private keys are not encrypted because otherwise the user is forced to enter their key at every startup or use, which is inconvenient and would punish users for using encryption and would leave their key more vulnerable to observation by malware or people around them. The entry of the key also becomes an important consent point: a user of an encrypted wallet can never accidentally send money no matter how stupidly they click or how far they explore.
Encryption of a wallet is not usually adequate for privacy on your local computer in any case: your logs, browser cache, etc all leak much of the same data... When we talk about backups the situation is somewhat different, and I can agree it may make sense to have all the data encrypted there. If I were to do it again I would have an master key encrypted with the hardened user passphrase used to derive separate keys for encrypting the whole wallet and the private keys. At the first use, I'd cache the whole wallet subkey (but not the private key one) in a separate file. This way backups would preserve privacy, so long as they didn't backup the cache.
BIP32 is not at all a replacement for backups; at most it protects key material. It does nothing to preserve metadata which can be absolutely critical (keeping in mind that in much of the developed world if your taxes are audited its up to you to prove you were paying what you owed). I somewhat regret inventing the public derivation thats most commonly used now-- it's overused, and in ways that harm security (often by people who do not understand the implications).
Closing for this older PR. It sounds like there is general agreement that something like this is a good direction, but it's doesn't seem to be coalescing into a specific solution that is liked.
My own criticism: I don't think this low level functionality is needed at all for non-GUI daemon. It can be implemented with a simple, external cron script that applies well to the local site.
Suggestions:
Agree with @jgarzik in general. What made me stop woking on this: as long as an "encrypted wallet" contains unencrypted sensitive data (like transactions [unencrypted], labels [unencrypted] and wtx comments [unencrypted]) it is to risky to auto-backup a wallet. Users might start to think they can store the backup in a insecure space (like dropbox, etc.) because they are assuming the backup is encrypted (which is a false assumption, only the private keys are encrypted).