Merge #11687: External wallet files

be8ab7d08 Create new wallet databases as directories rather than files (Russell Yanofsky)
26c06f24e Allow wallet files not in -walletdir directory (Russell Yanofsky)
d8a99f65e Allow wallet files in multiple directories (Russell Yanofsky)

Pull request description:

  This change consists of three commits:

  * The first commit is a pure refactoring that removes the restriction that two wallets can only be opened at the same time if they are contained in the same directory.
  * The second commit removes the restriction that `-wallet` filenames can only refer to files in the `-walletdir` directory.
  * The third commit makes second commit a little safer by changing bitcoin to create wallet databases as directories rather than files, so they can be safely backed up.

  All three commits should be straightforward:

  *  The first commit adds around 20 lines of new code and then updates a bunch of function signatures (generally updating them to take plain fs::path parameters, instead of combinations of strings, fs::paths, and objects like CDBEnv and CWalletDBWrapper).
  * The second commit removes two `-wallet` filename checks and adds some test cases to the multiwallet unit test.
  * The third commit just changes the mapping from specified wallet paths to bdb environment & data paths.

    ---

  **Note:** For anybody looking at this PR for the first time, I think you can skip the comments before _20 Nov_ and start reading at https://github.com/bitcoin/bitcoin/pull/11687#issuecomment-345625565. Comments before  _20 Nov_ were about an earlier version of the PR that didn't include the third commit, and then confusion from not seeing the first commit.

Tree-SHA512: 00bbb120fe0df847cf57014f75f1f7f1f58b0b62fa0b3adab4560163ebdfe06ccdfff33b4231693f03c5dc23601cb41954a07bcea9a4919c8d42f7d62bcf6024

2 files changed, 50 insertions, 37 deletions

diff --git a/test/functional/feature_config_args.py b/test/functional/feature_config_args.py
index 61abba8082..c6cec0596b 100755
--- a/test/functional/feature_config_args.py
+++ b/test/functional/feature_config_args.py
@@ -37,13 +37,13 @@ class ConfArgsTest(BitcoinTestFramework):
         os.mkdir(new_data_dir)
         self.start_node(0, ['-conf='+conf_file, '-wallet=w1'])
         self.stop_node(0)
-        assert os.path.isfile(os.path.join(new_data_dir, 'regtest', 'wallets', 'w1'))
+        assert os.path.exists(os.path.join(new_data_dir, 'regtest', 'wallets', 'w1'))
 
         # Ensure command line argument overrides datadir in conf
         os.mkdir(new_data_dir_2)
         self.nodes[0].datadir = new_data_dir_2
         self.start_node(0, ['-datadir='+new_data_dir_2, '-conf='+conf_file, '-wallet=w2'])
-        assert os.path.isfile(os.path.join(new_data_dir_2, 'regtest', 'wallets', 'w2'))
+        assert os.path.exists(os.path.join(new_data_dir_2, 'regtest', 'wallets', 'w2'))
 
 if __name__ == '__main__':
     ConfArgsTest().main()
diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py
index b07e451667..378c06ee59 100755
--- a/test/functional/wallet_multiwallet.py
+++ b/test/functional/wallet_multiwallet.py
@@ -16,7 +16,6 @@ class MultiWalletTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True
         self.num_nodes = 2
-        self.extra_args = [['-wallet=w1', '-wallet=w2', '-wallet=w3', '-wallet=w'], []]
         self.supports_cli = True
 
     def run_test(self):
@@ -26,9 +25,42 @@ class MultiWalletTest(BitcoinTestFramework):
         wallet_dir = lambda *p: data_dir('wallets', *p)
         wallet = lambda name: node.get_wallet_rpc(name)
 
-        assert_equal(set(node.listwallets()), {"w1", "w2", "w3", "w"})
-
+        # check wallet.dat is created
         self.stop_nodes()
+        assert_equal(os.path.isfile(wallet_dir('wallet.dat')), True)
+
+        # create symlink to verify wallet directory path can be referenced
+        # through symlink
+        os.mkdir(wallet_dir('w7'))
+        os.symlink('w7', wallet_dir('w7_symlink'))
+
+        # rename wallet.dat to make sure plain wallet file paths (as opposed to
+        # directory paths) can be loaded
+        os.rename(wallet_dir("wallet.dat"), wallet_dir("w8"))
+
+        # restart node with a mix of wallet names:
+        #   w1, w2, w3 - to verify new wallets created when non-existing paths specified
+        #   w          - to verify wallet name matching works when one wallet path is prefix of another
+        #   sub/w5     - to verify relative wallet path is created correctly
+        #   extern/w6  - to verify absolute wallet path is created correctly
+        #   w7_symlink - to verify symlinked wallet path is initialized correctly
+        #   w8         - to verify existing wallet file is loaded correctly
+        #   ''         - to verify default wallet file is created correctly
+        wallet_names = ['w1', 'w2', 'w3', 'w', 'sub/w5', os.path.join(self.options.tmpdir, 'extern/w6'), 'w7_symlink', 'w8', '']
+        extra_args = ['-wallet={}'.format(n) for n in wallet_names]
+        self.start_node(0, extra_args)
+        assert_equal(set(node.listwallets()), set(wallet_names))
+
+        # check that all requested wallets were created
+        self.stop_node(0)
+        for wallet_name in wallet_names:
+            if os.path.isdir(wallet_dir(wallet_name)):
+                assert_equal(os.path.isfile(wallet_dir(wallet_name, "wallet.dat")), True)
+            else:
+                assert_equal(os.path.isfile(wallet_dir(wallet_name)), True)
+
+        # should not initialize if wallet path can't be created
+        self.assert_start_raises_init_error(0, ['-wallet=wallet.dat/bad'], 'Not a directory')
 
         self.assert_start_raises_init_error(0, ['-walletdir=wallets'], 'Error: Specified -walletdir "wallets" does not exist')
         self.assert_start_raises_init_error(0, ['-walletdir=wallets'], 'Error: Specified -walletdir "wallets" is a relative path', cwd=data_dir())
@@ -37,17 +69,13 @@ class MultiWalletTest(BitcoinTestFramework):
         # should not initialize if there are duplicate wallets
         self.assert_start_raises_init_error(0, ['-wallet=w1', '-wallet=w1'], 'Error loading wallet w1. Duplicate -wallet filename specified.')
 
-        # should not initialize if wallet file is a directory
-        os.mkdir(wallet_dir('w11'))
-        self.assert_start_raises_init_error(0, ['-wallet=w11'], 'Error loading wallet w11. -wallet filename must be a regular file.')
-
         # should not initialize if one wallet is a copy of another
-        shutil.copyfile(wallet_dir('w2'), wallet_dir('w22'))
-        self.assert_start_raises_init_error(0, ['-wallet=w2', '-wallet=w22'], 'duplicates fileid')
+        shutil.copyfile(wallet_dir('w8'), wallet_dir('w8_copy'))
+        self.assert_start_raises_init_error(0, ['-wallet=w8', '-wallet=w8_copy'], 'duplicates fileid')
 
         # should not initialize if wallet file is a symlink
-        os.symlink(wallet_dir('w1'), wallet_dir('w12'))
-        self.assert_start_raises_init_error(0, ['-wallet=w12'], 'Error loading wallet w12. -wallet filename must be a regular file.')
+        os.symlink('w8', wallet_dir('w8_symlink'))
+        self.assert_start_raises_init_error(0, ['-wallet=w8_symlink'], 'Invalid -wallet path')
 
         # should not initialize if the specified walletdir does not exist
         self.assert_start_raises_init_error(0, ['-walletdir=bad'], 'Error: Specified -walletdir "bad" does not exist')
@@ -77,15 +105,17 @@ class MultiWalletTest(BitcoinTestFramework):
         self.restart_node(0, ['-walletdir='+competing_wallet_dir])
         self.assert_start_raises_init_error(1, ['-walletdir='+competing_wallet_dir], 'Error initializing wallet database environment')
 
-        self.restart_node(0, self.extra_args[0])
+        self.restart_node(0, extra_args)
 
-        w1 = wallet("w1")
-        w2 = wallet("w2")
-        w3 = wallet("w3")
-        w4 = wallet("w")
+        wallets = [wallet(w) for w in wallet_names]
         wallet_bad = wallet("bad")
 
-        w1.generate(1)
+        # check wallet names and balances
+        wallets[0].generate(1)
+        for wallet_name, wallet in zip(wallet_names, wallets):
+            info = wallet.getwalletinfo()
+            assert_equal(info['immature_balance'], 50 if wallet is wallets[0] else 0)
+            assert_equal(info['walletname'], wallet_name)
 
         # accessing invalid wallet fails
         assert_raises_rpc_error(-18, "Requested wallet does not exist or is not loaded", wallet_bad.getwalletinfo)
@@ -93,24 +123,7 @@ class MultiWalletTest(BitcoinTestFramework):
         # accessing wallet RPC without using wallet endpoint fails
         assert_raises_rpc_error(-19, "Wallet file not specified", node.getwalletinfo)
 
-        # check w1 wallet balance
-        w1_info = w1.getwalletinfo()
-        assert_equal(w1_info['immature_balance'], 50)
-        w1_name = w1_info['walletname']
-        assert_equal(w1_name, "w1")
-
-        # check w2 wallet balance
-        w2_info = w2.getwalletinfo()
-        assert_equal(w2_info['immature_balance'], 0)
-        w2_name = w2_info['walletname']
-        assert_equal(w2_name, "w2")
-
-        w3_name = w3.getwalletinfo()['walletname']
-        assert_equal(w3_name, "w3")
-
-        w4_name = w4.getwalletinfo()['walletname']
-        assert_equal(w4_name, "w")
-
+        w1, w2, w3, w4, *_ = wallets
         w1.generate(101)
         assert_equal(w1.getbalance(), 100)
         assert_equal(w2.getbalance(), 0)