let user select wallet file with -wallet=foo.dat#1889
let user select wallet file with -wallet=foo.dat#1889gavinandresen merged 1 commit intobitcoin:masterfrom lost-tty:multi-wallet
Conversation
src/main.cpp
Outdated
There was a problem hiding this comment.
I would feel better, if this would be a std::string.
There was a problem hiding this comment.
Me too, that would make it an owning reference to the underlying data. The memory referenced by .c_str() is deallocated when the underlying std::string goes out of scope, so after the Init() function. Keeping a global reference to it results in use-after-free problems...
|
Eh. The functionality is desirable, but the ability to filesystem split the wallet and data dir is a surefire way to end up with a corrupted wallet. This is subtle and I suspect hard to warn people out of doing, esp since it would mostly work. (until it eats your keys for lunch). |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/38ff7ee0239d1609a9bafd764f53fb45bbf3ac33 for binaries and test log. |
|
I agree with @gmaxwell. Can we add a check so only wallet names without / (or other filesystem separation character) are accepted? Maybe even limit to just alphanumeric names, and add ".dat" implicitly? |
|
Now that ultraprune has taken everything else out of the BDB environment, we can now have pluggable wallet locations, but it needs to move the whole db environment too, not just the wallet.dat location. |
|
This patch was never meant to be used to access wallets outside $DATADIR. I've written it to use multiple wallets within the same datadir. |
|
Maybe not, but then it should either enforce that requirement. Alternatively, it could allow accessing a wallet file anyway, as long as the BDB dir (not the entire datadir) is moved along. |
|
Without one of enforcement or moving the datadir it's a serious footgun that will result in permanent coin loss. I like moving the datadir, since it's viable now— and it would let you have your wallet file on a encrypted/removable volume while the chain resides on some less secure medium (or even a ramdisk). |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/9ccd2da8ad5a1c38db4d3bb73a8638fa6457533a for binaries and test log. |
|
@tcatm Any update and/or rebase? |
|
I guess in the light of real multiple wallet support coming along, this is not needed anymore? |
|
Agree with the other posts. This is very useful functionality, but this should either
Personally I like (2) most. |
|
As for (2) we really need a way to set another wallet directory (ie, walletdir=...). There is no reason anymore why the wallet directory and block chain directory would need to be the same. |
|
Having been using this for a few months, I have to say I prefer (1). |
|
Why? Are you against setting the wallet directory? |
|
It's easier and less disk-bloating to just use simple files, IMO. |
|
How do you mean "just use simple files"? I don't understand you, we aren't changing the wallet format here. BDB will still create a database environment, and it needs to be on the same disk as the .dat file (for reasons gmaxwell mentions). So if the user wants to store the wallet on say, a removable disk, he needs the database environment there as well. Option 1 would make this impossible (as the database environment is locked in place), and forces the user to always have the wallet in the same directory as the block chain. |
|
Different use cases / user preferences, I guess. Maybe if there's a "/" on the end, interpret it as a directory, and if there isn't, make sure there's no "/"s at all? |
|
Or maybe two separate options, instead of trying to parse it from one value: |
|
Too much shed painting; let's just merge it as long as the main concern is solved :) (prefer to append the file extension in code, to ease changing wallet formats later) |
|
I don't see the problem. Just allow specifying any filename, and use database/ in the directory that file is in as a database dir? If filename doesn't contain any /, use the datadir. |
|
Agree with @sipa |
|
I've updated this request:
|
|
ACK on the semantics, but can you move strWalletFile from main.cpp to init.cpp? (the validation logic has no business knowing where the wallet file is located). |
|
Any objections or ACKs? |
|
ACK, if you squeeze the commits. |
use std::string instead of psz for WalletFile only allow wallets within $DATADIR Use strWalletFile in salvage/recover fix: remove unused variable pszWalletFile move strWalletFile to init.h/init.cpp avoid conversion of strWalletfile to c-string
|
ACK |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/674cb304b376358fdcb17b4a0b16ae7b00cdbedc for binaries and test log. |
let user select wallet file with -wallet=foo.dat
… extraction. b189cc3 Created coldstaking_tests unit test, checking cold staking script keys proper extraction. (furszy) Pull request description: Essentially proving bitcoin#1830 bug and bitcoin#1888 subsequent fix. ACKs for top commit: Fuzzbawls: ACK b189cc3 random-zebra: ACK b189cc3 (failing on master, passing on bitcoin#1888). Merging... Tree-SHA512: 9af3975fe099a0e26feddf2fd52abee12864bf77bb511b2fbed6f39a9ea029cdc057d15157fafe842cd7e2b438447a08dc455c763feeecdc1378af637541eaf1
8 participants
This patch lets a user have multiple wallets within his data directory by selecting a wallet using
-wallet=filename. It also allows accessing wallets outside the data directory. In that case, the database environment is still stored in the data directory so both wallet file and data directory must be kept in sync (e.g. when storing the wallet within an encrypted container).I've been using this patch for a few months now without any troubles and it's proved to be pretty useful.