Navigation Menu

Skip to content
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

Jump to bottom

Bump wallet version to 159900 and remove the usehd option #11250

Merged
merged 2 commits into from Sep 8, 2017
Merged

Bump wallet version to 159900 and remove the usehd option #11250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d4c18f7
Bump wallet version number to 159900
achow101 Sep 5, 2017
713a920
Remove usehd option and warn when it is used
achow101 Sep 5, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion src/wallet/init.cpp
View file
Open in desktop
Expand Up @@ -29,7 +29,6 @@ std::string GetWalletHelpString(bool showDebug)
strUsage += HelpMessageOpt("-salvagewallet", _("Attempt to recover private keys from a corrupt wallet on startup"));
strUsage += HelpMessageOpt("-spendzeroconfchange", strprintf(_("Spend unconfirmed change when sending transactions (default: %u)"), DEFAULT_SPEND_ZEROCONF_CHANGE));
strUsage += HelpMessageOpt("-txconfirmtarget=<n>", strprintf(_("If paytxfee is not set, include enough fee so transactions begin confirmation on average within n blocks (default: %u)"), DEFAULT_TX_CONFIRM_TARGET));
strUsage += HelpMessageOpt("-usehd", _("Use hierarchical deterministic key generation (HD) after BIP32. Only has effect during wallet creation/first start") + " " + strprintf(_("(default: %u)"), DEFAULT_USE_HD_WALLET));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs mention in the release notes and fixup for check-doc.py

strUsage += HelpMessageOpt("-walletrbf", strprintf(_("Send transactions with full-RBF opt-in enabled (default: %u)"), DEFAULT_WALLET_RBF));
strUsage += HelpMessageOpt("-upgradewallet", _("Upgrade wallet to latest format on startup"));
strUsage += HelpMessageOpt("-wallet=<file>", _("Specify wallet file (within data directory)") + " " + strprintf(_("(default: %s)"), DEFAULT_WALLET_DAT));
Expand Down
18 changes: 7 additions & 11 deletions src/wallet/wallet.cpp
View file
Open in desktop
Expand Up @@ -3829,17 +3829,13 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)

if (fFirstRun)
{
// Create new keyUser and set as default key
if (gArgs.GetBoolArg("-usehd", DEFAULT_USE_HD_WALLET) && !walletInstance->IsHDEnabled()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussion I believe the removal of the !IsHDEnabled check here is fine but I could totally see it breaking something very subtly, so would be good to get more eyeballs on this line.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the IsHDEnabled() check was there because you could salvage the wallet which may then would have triggered a fFirstRun and recreated a HD master key. But this is no longer true since fFirstRun is always false if there is a key in the wallet (and HD master key is also a standard key).

// ensure this wallet.dat can only be opened by clients supporting HD with chain split and expects no default key
walletInstance->SetMinVersion(FEATURE_NO_DEFAULT_KEY);

// ensure this wallet.dat can only be opened by clients supporting HD with chain split
walletInstance->SetMinVersion(FEATURE_HD_SPLIT);

// generate a new master key
CPubKey masterPubKey = walletInstance->GenerateNewHDMasterKey();
if (!walletInstance->SetHDMasterKey(masterPubKey))
throw std::runtime_error(std::string(__func__) + ": Storing master key failed");
}
// generate a new master key
CPubKey masterPubKey = walletInstance->GenerateNewHDMasterKey();
if (!walletInstance->SetHDMasterKey(masterPubKey))
throw std::runtime_error(std::string(__func__) + ": Storing master key failed");

// Top up the keypool
if (!walletInstance->TopUpKeyPool()) {
Expand All @@ -3852,7 +3848,7 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
else if (gArgs.IsArgSet("-usehd")) {
bool useHD = gArgs.GetBoolArg("-usehd", DEFAULT_USE_HD_WALLET);
if (walletInstance->IsHDEnabled() && !useHD) {
InitError(strprintf(_("Error loading %s: You can't disable HD on an already existing HD wallet"), walletFile));
InitError(strprintf(_("Error loading %s: You can't disable HD on an already existing HD wallet or create new non-HD wallets."), walletFile));
Copy link
Member

@maflcko maflcko Sep 10, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new warning will only be printed in subsequent loads of the wallet (after the first start) since the code branch is never executed on first start.

return nullptr;
}
if (!walletInstance->IsHDEnabled() && useHD) {
Expand Down
2 changes: 2 additions & 0 deletions src/wallet/wallet.h
View file
Open in desktop
Expand Up @@ -96,6 +96,8 @@ enum WalletFeature

FEATURE_HD_SPLIT = 139900, // Wallet with HD chain split (change outputs will use m/0'/1'/k)

FEATURE_NO_DEFAULT_KEY = 159900, // Wallet without a default key written

FEATURE_LATEST = FEATURE_COMPRPUBKEY // HD is optional, use FEATURE_COMPRPUBKEY as latest version
};

Expand Down
2 changes: 1 addition & 1 deletion test/functional/keypool-topup.py
View file
Open in desktop
Expand Up @@ -23,7 +23,7 @@ class KeypoolRestoreTest(BitcoinTestFramework):
def set_test_params(self):
self.setup_clean_chain = True
self.num_nodes = 2
self.extra_args = [['-usehd=0'], ['-usehd=1', '-keypool=100', '-keypoolmin=20']]
self.extra_args = [[], ['-keypool=100', '-keypoolmin=20']]

def run_test(self):
self.tmpdir = self.options.tmpdir
Expand Down
8 changes: 1 addition & 7 deletions test/functional/wallet-hd.py
View file
Open in desktop
Expand Up @@ -15,17 +15,11 @@ class WalletHDTest(BitcoinTestFramework):
def set_test_params(self):
self.setup_clean_chain = True
self.num_nodes = 2
self.extra_args = [['-usehd=0'], ['-usehd=1', '-keypool=0']]
self.extra_args = [[], ['-keypool=0']]

def run_test (self):
tmpdir = self.options.tmpdir

# Make sure can't switch off usehd after wallet creation
self.stop_node(1)
self.assert_start_raises_init_error(1, ['-usehd=0'], 'already existing HD wallet')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this test removed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. That was removed originally when I had -usehd always return an error regardless.

self.start_node(1)
connect_nodes_bi(self.nodes, 0, 1)

# Make sure we use hd, keep masterkeyid
masterkeyid = self.nodes[1].getwalletinfo()['hdmasterkeyid']
assert_equal(len(masterkeyid), 40)
Expand Down
3 changes: 1 addition & 2 deletions test/functional/wallet.py
View file
Open in desktop
Expand Up @@ -10,10 +10,9 @@ class WalletTest(BitcoinTestFramework):
def set_test_params(self):
self.num_nodes = 4
self.setup_clean_chain = True
self.extra_args = [['-usehd={:d}'.format(i%2==0)] for i in range(4)]

def setup_network(self):
self.add_nodes(4, self.extra_args)
self.add_nodes(4)
self.start_node(0)
self.start_node(1)
self.start_node(2)
Expand Down