Merge bitcoin#1410: [Tests] Run tests on pre-HD wallets

4cd5c51 [Tests] Fix wallet_upgrade (random-zebra)
58ff9ec [Tests] Fix wallet_keypool_topup.py for non-HD wallets (random-zebra)
4cc90d0 [Tests] Add -legacywallet flag to run functional tests on pre-HD wallets (random-zebra)
d1b070c [Wallet] Remove check preventing legacy wallet keypool topup (random-zebra)
a22609d [Init] Add -legacywallet startup option (random-zebra)

Pull request description:

  This adds a startup flag `-legacywallet`, which, on first run, enables the creation of a pre-HD wallet (when a wallet.dat is already present, the flag is just ignored). This flag is available for RegTest only.

  A special option `--legacywallet` is also added to the functional test_runner.
  Single tests (or the entire test suite) can now be performed over legacy wallets easily, e.g.:
  ```
  ./test_runner.py -j10 --legacywallet

  ./wallet_basic.py --legacywallet
  ```

  **Note 1**: Running with pre-HD wallets, the `wallet_accounts.py` test fails. It is fixed in a successive PR.

  **Note 2**: Travis currently is set to run the functional test suite only on HD wallets.

Top commit has no ACKs.

Tree-SHA512: 311db59078ee3c5bd727eaa6fa9f3e19a5490bc42359d8c849b06da617376898644b6cb93ab8cbbd4deb515111ff165ba0478f74371fb9bd240dc74006cda9c9

src/init.cpp

@@ -470,6 +470,7 @@ std::string HelpMessage(HelpMessageMode mode)
     strUsage += HelpMessageOpt("-txconfirmtarget=<n>", strprintf(_("If paytxfee is not set, include enough fee so transactions begin confirmation on average within n blocks (default: %u)"), 1));
     strUsage += HelpMessageOpt("-maxtxfee=<amt>", strprintf(_("Maximum total fees to use in a single wallet transaction, setting too low may abort large transactions (default: %s)"),
         FormatMoney(maxTxFee)));
+    strUsage += HelpMessageOpt("-legacywallet", _("On first run, create a legacy wallet instead of a HD wallet"));
     strUsage += HelpMessageOpt("-upgradewallet", _("Upgrade wallet to latest format") + " " + _("on startup"));
     strUsage += HelpMessageOpt("-wallet=<file>", _("Specify wallet file (within data directory)") + " " + strprintf(_("(default: %s)"), "wallet.dat"));
     strUsage += HelpMessageOpt("-walletnotify=<cmd>", _("Execute command when a wallet transaction changes (%s in cmd is replaced by TxID)"));
@@ -1681,28 +1682,29 @@ bool AppInit2()
         }
 
         int prev_version = pwalletMain->GetVersion();
-        if (GetBoolArg("-upgradewallet", fFirstRun)) {
 
+        // Forced upgrade
+        const bool fLegacyWallet = GetBoolArg("-legacywallet", false);
+        if (GetBoolArg("-upgradewallet", fFirstRun && !fLegacyWallet)) {
             if (prev_version <= FEATURE_PRE_PIVX && pwalletMain->IsLocked()) {
                 // Cannot upgrade a locked wallet
                 std::string strProblem = "Cannot upgrade a locked wallet.\n";
                 strErrors << _("Error: ") << strProblem;
                 LogPrintf("%s", strErrors.str());
                 return InitError(strProblem);
-            } else {
-
-                int nMaxVersion = GetArg("-upgradewallet", 0);
-                if (nMaxVersion == 0) // the -upgradewallet without argument case
-                {
-                    LogPrintf("Performing wallet upgrade to %i\n", FEATURE_LATEST);
-                    nMaxVersion = FEATURE_LATEST;
-                    pwalletMain->SetMinVersion(FEATURE_LATEST); // permanently upgrade the wallet immediately
-                } else
-                    LogPrintf("Allowing wallet upgrade up to %i\n", nMaxVersion);
-                if (nMaxVersion < pwalletMain->GetVersion())
-                    strErrors << _("Cannot downgrade wallet") << "\n";
-                pwalletMain->SetMaxVersion(nMaxVersion);
             }
+
+            int nMaxVersion = GetArg("-upgradewallet", 0);
+            if (nMaxVersion == 0) // the -upgradewallet without argument case
+            {
+                LogPrintf("Performing wallet upgrade to %i\n", FEATURE_LATEST);
+                nMaxVersion = FEATURE_LATEST;
+                pwalletMain->SetMinVersion(FEATURE_LATEST); // permanently upgrade the wallet immediately
+            } else
+                LogPrintf("Allowing wallet upgrade up to %i\n", nMaxVersion);
+            if (nMaxVersion < pwalletMain->GetVersion())
+                strErrors << _("Cannot downgrade wallet") << "\n";
+            pwalletMain->SetMaxVersion(nMaxVersion);
         }
 
         // Upgrade to HD only if explicit upgrade was requested
@@ -1714,11 +1716,23 @@ bool AppInit2()
         }
 
         if (fFirstRun) {
-            // Create new HD Wallet
-            LogPrintf("Creating HD Wallet\n");
-            // Ensure this wallet.dat can only be opened by clients supporting HD.
-            pwalletMain->SetMinVersion(FEATURE_LATEST);
-            pwalletMain->SetupSPKM();
+            if (!fLegacyWallet) {
+                // Create new HD Wallet
+                LogPrintf("Creating HD Wallet\n");
+                // Ensure this wallet.dat can only be opened by clients supporting HD.
+                pwalletMain->SetMinVersion(FEATURE_LATEST);
+                pwalletMain->SetupSPKM();
+            } else {
+                if (!Params().IsRegTestNet()) {
+                    std::string strProblem = "Legacy wallets can only be created on RegTest.\n";
+                    strErrors << _("Error: ") << strProblem;
+                    LogPrintf("%s", strErrors.str());
+                    return InitError(strProblem);
+                }
+                // Create legacy wallet
+                LogPrintf("Creating Pre-HD Wallet\n");
+                pwalletMain->SetMaxVersion(FEATURE_PRE_PIVX);
+            }
 
             // Top up the keypool
             if (!pwalletMain->TopUpKeyPool()) {

src/wallet/scriptpubkeyman.cpp

@@ -361,9 +361,6 @@ bool ScriptPubKeyMan::NewKeyPool()
  */
 bool ScriptPubKeyMan::TopUp(unsigned int kpSize)
 {
-    if (!CanGenerateKeys()) {
-        return false;
-    }
     {
         LOCK(wallet->cs_wallet);
         if (wallet->IsLocked()) return false;

test/functional/test_framework/test_framework.py

@@ -122,6 +122,8 @@ def main(self):
                           help="Write tested RPC commands into this directory")
         parser.add_option("--configfile", dest="configfile",
                           help="Location of the test framework config file")
+        parser.add_option('--legacywallet', dest="legacywallet", default=False, action="store_true",
+                          help='create pre-HD wallets only')
         parser.add_option("--pdbonfailure", dest="pdbonfailure", default=False, action="store_true",
                           help="Attach a python debugger if test fails")
         parser.add_option("--usecli", dest="usecli", default=False, action="store_true",
@@ -248,6 +250,11 @@ def add_nodes(self, num_nodes, extra_args=None, rpchost=None, timewait=None, bin
 
         if extra_args is None:
             extra_args = [[]] * num_nodes
+        # Check wallet version
+        if self.options.legacywallet:
+            for arg in extra_args:
+                arg.append('-legacywallet')
+            self.log.info("Running test with legacy (pre-HD) wallet")
         if binary is None:
             binary = [None] * num_nodes
         assert_equal(len(extra_args), num_nodes)

test/functional/test_runner.py

@@ -177,6 +177,7 @@ def main():
     parser.add_argument('--jobs', '-j', type=int, default=4, help='how many test scripts to run in parallel. Default=4.')
     parser.add_argument('--keepcache', '-k', action='store_true', help='the default behavior is to flush the cache directory on startup. --keepcache retains the cache from the previous testrun.')
     parser.add_argument('--quiet', '-q', action='store_true', help='only print dots, results summary and failure logs')
+    parser.add_argument('--legacywallet', '-w', action='store_true', help='create pre-HD wallets only')
     parser.add_argument('--tmpdirprefix', '-t', default=tempfile.gettempdir(), help="Root directory for datadirs")
     args, unknown_args = parser.parse_known_args()
 
@@ -190,6 +191,8 @@ def main():
     config.read_file(open(configfile))
 
     passon_args.append("--configfile=%s" % configfile)
+    if args.legacywallet:
+        passon_args.append("--legacywallet")
 
     # Set up logging
     logging_level = logging.INFO if args.quiet else logging.DEBUG

test/functional/wallet_hd.py

@@ -26,7 +26,11 @@ def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()
 
     def run_test(self):
-        # Make sure we use hd, keep masterkeyid
+        # Make sure we use hd
+        if '-legacywallet' in self.nodes[0].extra_args:
+            self.log.info("Exiting HD test for non-HD wallets")
+            return
+        # keep masterkeyid
         masterkeyid = self.nodes[1].getwalletinfo()['hdseedid']
         assert_equal(len(masterkeyid), 40)
 

test/functional/wallet_keypool_topup.py

@@ -26,6 +26,7 @@ def set_test_params(self):
         self.extra_args = [['-keypool=3'], ['-keypool=100']]
 
     def run_test(self):
+        isLegacyWallet = '-legacywallet' in self.nodes[0].extra_args
         self.tmpdir = self.options.tmpdir
         self.nodes[0].generate(101)
 
@@ -64,11 +65,15 @@ def run_test(self):
         connect_nodes_bi(self.nodes, 0, 1)
         self.sync_all()
 
-        assert_equal(self.nodes[1].getbalance(), 15)
+        # wallet was not backupped after emptying the key pool.
+        # Legacy wallet can't recover funds in addr_extpool
+        recoveredBalance = 10 if isLegacyWallet else 15
+        assert_equal(self.nodes[1].getbalance(), recoveredBalance)
         assert_equal(self.nodes[1].listtransactions()[0]['category'], "receive")
 
         # Check that we have marked all keys up to the used keypool key as used
-        assert_equal(self.nodes[1].getaddressinfo(self.nodes[1].getnewaddress())['hdkeypath'], "m/44'/119'/0'/0'/110'")
+        if not isLegacyWallet:
+            assert_equal(self.nodes[1].getaddressinfo(self.nodes[1].getnewaddress())['hdkeypath'], "m/44'/119'/0'/0'/110'")
 
 if __name__ == '__main__':
     KeypoolRestoreTest().main()

test/functional/wallet_upgrade.py

@@ -76,7 +76,7 @@ def set_test_params(self):
         self.setup_clean_chain = True
         self.num_nodes = 1
 
-    def check_keys(self, addrs):
+    def check_keys(self, addrs, mined_blocks = 0):
         self.log.info("Checking old keys existence in the upgraded wallet..")
         # Now check that all of the pre upgrade addresses are still in the wallet
         for addr in addrs:
@@ -87,7 +87,7 @@ def check_keys(self, addrs):
         self.log.info("All pre-upgrade keys found in the wallet :)")
 
         # Use all of the keys in the pre-HD keypool
-        for _ in range(0, 60):
+        for _ in range(0, 60 + mined_blocks):
             self.nodes[0].getnewaddress()
 
         self.log.info("All pre-upgrade keys should have been marked as used by now, creating new HD keys")
@@ -101,6 +101,10 @@ def check_keys(self, addrs):
 
 
     def run_test(self):
+        # Make sure we use hd
+        if '-legacywallet' in self.nodes[0].extra_args:
+            self.log.info("Exiting HD upgrade test for non-HD wallets")
+            return
         self.log.info("Checking correct version")
         assert_equal(self.nodes[0].getwalletinfo()['walletversion'], 61000)
 
@@ -139,7 +143,7 @@ def run_test(self):
 
         self.log.info("upgrade completed, checking keys now..")
         # Now check if the upgrade went fine
-        self.check_keys(addrs)
+        self.check_keys(addrs, 1)
 
         self.log.info("Upgrade via RPC completed, all good :)")