available field to listwallets RPC
#11485
Modifies listwallets to return a list of ‘available’ wallets in the wallet directory, by looking for the BDB magic bytes (0x00053162 source: https://github.com/file/file/blob/master/magic/Magdir/database) .
This is a breaking change to listwallets, but multiwallet RPC calls are experimental in 0.15 so shouldn’t be a big concern to change it. c.f. @jnewbery’s comment here. Split from #11466.
Would be great if someone could confirm if this is endianness independent too :)
As said in #11466 (review), a different interface is:
0{
1 "wallets": [
2 { "name": "foo", "loaded": true },
3 { "name": "bar", "loaded": false }
4 ]
5}
Which has the advantage of allowing to add new attributes later.
Another option is to have a different RPC. So:
listwallets - returns a lists of active/loaded walletsfindwallets - returns a list of all possible/available wallets.This has the advantage that each RPC has it’s own purpose and implementation, which happen to be very different. For those that manage (keep record of) the wallets externally findwallets is of no interest, only listwallets.
2656+ for (fs::directory_iterator it(walletdir); it != fs::directory_iterator(); ++it) {
2657+ std::ifstream file(it->path().string(), std::ios::binary);
2658+ file.seekg(12, std::ios::beg); // Magic bytes start at offset 12
2659+ unsigned char fileBytes[4] = {0};
2660+ file.read((char*)fileBytes, sizeof(fileBytes)); // Read 4 bytes of file to compare against magic
2661+ if (is_regular_file(*it) && file.is_open() && memcmp(fileBytes, bdb_magic, sizeof(bdb_magic)) == 0) {
Please explicitly use fs::is_regular_file() (this only compiles because of Argument Dependant Lookups http://en.cppreference.com/w/cpp/language/adl which makes it difficult to understand where this function is coming from).
boost::filesystem::is_regular_file() will return true if called on a symlink to a regular file. We don’t want those to show up in the available list since they can’t be loaded as a wallet, so you should extend this if test with fs::is_symlink().
Looks good. Can you extend the checking to exclude symlinks from the available wallets list, and add a test to cover that?
I think it might also be a good idea to exclude currently loaded wallets from the available list (ie the loaded and available lists should be mutually exclusive)
I also tested manually that files show up as wallets when bytes 12-15 match the magic bdb bytes:
0→ bitcoin-cli listwallets
1{
2 "loaded": [
3 "wallet.dat"
4 ],
5 "available": [
6 "wallet.dat"
7 ]
8}
9[ubuntu] /home/ubuntu/.bitcoin/regtest
10→ bitcoin-cli stop
11Bitcoin server stopping
12[ubuntu] /home/ubuntu/.bitcoin/regtest
13→ for i in {1..20}; do head wallet.dat -c $i > wallet_prefix_$i; done
14[ubuntu] /home/ubuntu/.bitcoin/regtest
15→ bitcoind
16Bitcoin server starting
17[ubuntu] /home/ubuntu/.bitcoin/regtest
18→ bitcoin-cli listwallets
19{
20 "loaded": [
21 "wallet.dat"
22 ],
23 "available": [
24 "wallet_prefix_20",
25 "wallet_prefix_19",
26 "wallet.dat",
27 "wallet_prefix_16",
28 "wallet_prefix_17",
29 "wallet_prefix_15",
30 "wallet_prefix_18"
31 ]
32}
2644@@ -2645,10 +2645,38 @@ UniValue listwallets(const JSONRPCRequest& request)
2645
2646 LOCK(pwallet->cs_wallet);
2647
2648- obj.push_back(pwallet->GetName());
2649+ loaded.push_back(pwallet->GetName());
2650 }
Suggestion to remove loop below:
0std::set<std::string> loaded_set;
1...
2 loaded.push_back(pwallet->GetName());
3 loaded_set.insert(pwallet->GetName())
4}
5...
6if (loaded_set.count(it->path().filename().string()) == 0) {
7 available.push_back(it->path().filename().string());
8}
2661+ file.seekg(12, std::ios::beg); // Magic bytes start at offset 12
2662+ unsigned char fileBytes[4] = {0};
2663+
2664+ file.read((char*)fileBytes, sizeof(fileBytes)); // Read 4 bytes of file to compare against magic
2665+
2666+ if (fs::is_regular_file(*it) && !fs::is_symlink(*it) && file.is_open() && memcmp(fileBytes, bdb_magic, sizeof(bdb_magic)) == 0) {
Some of these conditions can be moved before seekg (just continue. Only the compare is needed.
Could use 64 bit int instead?
2672+ if (loaded_set.count(it->path().filename().string()) == 0) {
2673+ available.push_back(it->path().filename().string());
2674+ }
2675+ }
2676+
2677+ file.close();
2665+
2666+ file.seekg(12, std::ios::beg); // Magic bytes start at offset 12
2667+ unsigned char fileBytes[4] = {0};
2668+ file.read((char*)fileBytes, sizeof(fileBytes)); // Read 4 bytes of file to compare against magic
2669+
2670+ if (memcmp(fileBytes, bdb_magic, sizeof(bdb_magic)) == 0) {
Alternative, avoid nesting:
0if (memcmp(...) != 0) continue;
1
2if (loaded_set.count(filename) > 0) continue;
3
4available.push_back(filename);
Note that close is automatically called, see comment below.
Nit, add:
0const std::string filename = it->path().filename().string();
2662+
2663+ std::ifstream file(it->path().string(), std::ios::binary);
2664+ if(!file.is_open()) continue;
2665+
2666+ file.seekg(12, std::ios::beg); // Magic bytes start at offset 12
2667+ unsigned char fileBytes[4] = {0};
file_bytes.
2655+ UniValue available(UniValue::VARR);
2656+ fs::path walletdir = GetDataDir();
2657+ const unsigned char bdb_magic[4] = {0x62, 0x31, 0x05, 0x00}; // Berkeley DB Btree magic bytes
2658+
2659+ for (fs::directory_iterator it(walletdir); it != fs::directory_iterator(); ++it) {
2660+
18@@ -19,7 +19,7 @@ def set_test_params(self):
19 self.extra_args = [['-wallet=w1', '-wallet=w2', '-wallet=w3']]
20
21 def run_test(self):
22- assert_equal(set(self.nodes[0].listwallets()), {"w1", "w2", "w3"})
23+ assert_equal(set(self.nodes[0].listwallets()['loaded']), {"w1", "w2", "w3"})
24
Needs rebase, but I assume this won’t work after #11687 anyway?
It won’t work if someone uses external wallets, but it should still work if they have all wallets in the wallet directory? (which is the default)
63@@ -63,6 +64,10 @@ def run_test(self):
64
65 self.start_node(0, self.extra_args[0])
66
67+ # ensure listwallets only names BDB files, not symlinks, other files or directories
I like this new test, but I think it can be improved in a couple of ways:
What do you think about:
0--- a/test/functional/multiwallet.py
1+++ b/test/functional/multiwallet.py
2@@ -65,8 +65,20 @@ class MultiWalletTest(BitcoinTestFramework):
3 self.start_node(0, self.extra_args[0])
4
5 # ensure listwallets only names BDB files, not symlinks, other files or directories
6- open(os.path.join(wallet_dir, 'w01.dat'), 'a').close()
7- assert_equal(set(self.nodes[0].listwallets()['available']), {"w22"})
8+ with open(os.path.join(wallet_dir, 'w01.dat'), 'a') as wallet_file:
9+ # Create a file containing a long string
10+ print("a" * 2000, file=wallet_file)
11+
12+ available_wallets = self.nodes[0].listwallets()['available']
13+
14+ assert os.path.islink(os.path.join(wallet_dir, 'w12'))
15+ assert 'w12' not in available_wallets
16+
17+ assert os.path.isdir(os.path.join(wallet_dir, 'w11'))
18+ assert 'w11' not in available_wallets
19+
20+ assert os.path.isfile(os.path.join(wallet_dir, 'w01.dat'))
21+ assert 'w01.dat' not in available_wallets
2748@@ -2749,16 +2749,17 @@ UniValue listwallets(const JSONRPCRequest& request)
2749 "Returns a list of currently loaded wallets.\n"
2750 "For full information on the wallet, use \"getwalletinfo\"\n"
2751 "\nResult:\n"
2752- "[ (json array of strings)\n"
2753- " \"walletname\" (string) the wallet name\n"
2754- " ...\n"
2755- "]\n"
2756+ "{\n"
90@@ -91,6 +91,10 @@ Low-level RPC changes
91 - The wallet RPC `getreceivedbyaddress` will return an error if called with an address not in the wallet.
92
93
94+- `listwallets` now returns two arrays of wallet names, one listing the `loaded` wallets
I don’t think it’s a big deal to break backwards compatibility here, but I also don’t see any benefit in doing so. I’d think that a better approach to overloading the listwallets function would be to add a new listwalletdir function that would return a list of wallet files inside -walletdir. Advantages to this approach:
listwalletdir naming is consistent with recently added walletdir parameter, and tells you what the function actually does.2753- " \"walletname\" (string) the wallet name\n"
2754- " ...\n"
2755- "]\n"
2756+ "{\n"
2757+ " \"loaded\": [ \"walletname\" ... ] (json array of strings) Names of loaded wallets\n"
2758+ " \"available\": [ \"walletname\" ... ] (json array of strings) Names of BDB files in wallet directory (may not always actually be wallets)\n"
listwallets could have an optional parameter to filter available, loaded or both.
2780+ const unsigned char bdb_magic[4] = {0x62, 0x31, 0x05, 0x00}; // Berkeley DB Btree magic bytes
2781+
2782+ for (fs::directory_iterator it(walletdir); it != fs::directory_iterator(); ++it) {
2783+ if (!fs::is_regular_file(*it) || fs::is_symlink(*it)) continue;
2784+
2785+ std::ifstream file(it->path().string(), std::ios::binary);
Shame this has gone stale. I think it’s useful new functionality.
I’d be very happy to re-review if someone picked this up again.
I’m tempted to pick this as this is also useful in the UI — must be exposed in interfaces::Nodes.
As suggested by @Sjors in #13100#pullrequestreview-135815243
For opening an existing wallet I have a light preference for just scanning the default directory for wallets and showing a list of wallets to choose from. This also prevents confusion around wallets that are files vs. wallets that are directories. Offering multiple wallets and offering the ability to put wallets anywhere in the filesystem are two different features.
Thoughts?
