-
Notifications
You must be signed in to change notification settings - Fork 37.6k
wallet: Display non-HD error on first run #11307
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
Conversation
utACK fadf31e |
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
utACK fadf31e
@@ -3843,9 +3847,9 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile) | |||
walletInstance->SetBestChain(chainActive.GetLocator()); | |||
} | |||
else if (gArgs.IsArgSet("-usehd")) { |
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.
Instead of moving the code block up, couldn't this just be made an if
instead of else if
? If we want to show this error before doing any of the wallet creating stuff, this entire check could just be moved to before the first run block.
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.
Yes, the else can be removed for better clarity. However, I'd still prefer that the new-non-hd-error is guarded by first-run. This way we can return early and don't have to deal with TopUpKeyPool
, which might take a "long" time...
I don't think this whole check can be moved to before the first run block, as IsHDEnabled
should be called after (a potential) SetHDMasterKey
.
utACK fadf31e |
@@ -3827,6 +3827,10 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile) | |||
if (fFirstRun) | |||
{ | |||
// ensure this wallet.dat can only be opened by clients supporting HD with chain split and expects no default key | |||
if (!gArgs.GetBoolArg("-usehd", true)) { | |||
InitError(strprintf(_("Error creating %s: You can't create non-HD wallets with this version."), walletFile)); |
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.
Missing test?
@@ -3827,6 +3827,10 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile) | |||
if (fFirstRun) | |||
{ | |||
// ensure this wallet.dat can only be opened by clients supporting HD with chain split and expects no default key | |||
if (!gArgs.GetBoolArg("-usehd", true)) { | |||
InitError(strprintf(_("Error creating %s: You can't create non-HD wallets with this version."), walletFile)); | |||
return nullptr; |
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.
Is it me or walletInstance
is leaked?
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.
I believe it is, but that is not a problem since the program just exits here. The same behavior happens earlier too with other InitError
s.
This needs a 0.16.0 milestone |
utACK fadf31e Added 0.16 milestone. |
is there a link to the relevant issue? |
@instagibbs You can try it yourself with -usehd=0 or see #11313 |
Thanks, having the relevant issue in the PR description helps. |
fadf31e wallet: Display non-HD error on first run (MarcoFalke) Pull request description: On current master a fresh wallet created with `-usehd=0` is silently created as HD wallet. An error should be displayed on the first run. Also, this restores a test that was removed in c22a53c Fixes: #11313 Tree-SHA512: 226a4129984324f88a431c7e2726383f6841711f0227d8e9f5b4f89d4bb9f2b8e922e6cf0a6f91d6efa747d139543a236b9f29326fc5d1e5d6f1dea2465d9b85
On current master a fresh wallet created with
-usehd=0
is silently created as HD wallet.An error should be displayed on the first run.
Also, this restores a test that was removed in c22a53c
Fixes: #11313