Add available field to listwallets RPC

[meshcollider]

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 :)

[promag]

As said in #11466 (comment), a different interface is:

{
  "wallets": [
    { "name": "foo", "loaded": true },
    { "name": "bar", "loaded": false }
  ]
}

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 wallets
• findwallets - 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.

[jnewbery]

concept ACK splitting this from #11466. And concept ACK using the BDB magic bytes rather than filename

[meshcollider]

Removed WIP, now uses BDB magic bytes, although I haven't confirmed that this works on big endian or also potentially on old versions of BDB wallets if the magic bytes changed

[jnewbery]

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)

[jnewbery]

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().

[jnewbery]

I also tested manually that files show up as wallets when bytes 12-15 match the magic bdb bytes:

→ bitcoin-cli listwallets
{
  "loaded": [
    "wallet.dat"
  ],
  "available": [
    "wallet.dat"
  ]
}
[ubuntu] /home/ubuntu/.bitcoin/regtest
→ bitcoin-cli stop
Bitcoin server stopping
[ubuntu] /home/ubuntu/.bitcoin/regtest
→ for i in {1..20}; do head wallet.dat -c $i > wallet_prefix_$i; done
[ubuntu] /home/ubuntu/.bitcoin/regtest
→ bitcoind
Bitcoin server starting
[ubuntu] /home/ubuntu/.bitcoin/regtest
→ bitcoin-cli listwallets
{
  "loaded": [
    "wallet.dat"
  ],
  "available": [
    "wallet_prefix_20",
    "wallet_prefix_19",
    "wallet.dat",
    "wallet_prefix_16",
    "wallet_prefix_17",
    "wallet_prefix_15",
    "wallet_prefix_18"
  ]
}

[meshcollider]

Addressed @jnewbery nits and made available list mutually exclusive to loaded list. Thanks for the review :-)

[promag]

Suggestion to remove loop below:

std::set<std::string> loaded_set;
...
    loaded.push_back(pwallet->GetName());
    loaded_set.insert(pwallet->GetName())
}
...
if (loaded_set.count(it->path().filename().string()) == 0) {
    available.push_back(it->path().filename().string());
}

[meshcollider]

Good idea @promag, thanks :-)

[promag]

Some of these conditions can be moved before seekg (just continue. Only the compare is needed.

Could use 64 bit int instead?

[meshcollider]

Fixed the conditional, but IMO the char array is clearer and perhaps safer than using an int :)

[meshcollider]

Rebased on master to fix travis failure due to multiwallet.py being modified

[promag]

Remove, see https://stackoverflow.com/a/748059.

[promag]

Alternative, avoid nesting:

if (memcmp(...) != 0) continue;

if (loaded_set.count(filename) > 0) continue;

available.push_back(filename);

Note that close is automatically called, see comment below.

[promag]

Nit, add:

const std::string filename = it->path().filename().string();

[promag]

Nit: file_bytes.

[promag]

Nit, remove empty line.

[promag]

Assert available is empty array?

[meshcollider]

Fixed @promag's nits, thanks :)

BTW it would be good to have #11466 reviewed first because this is only really useful with that IMO

[meshcollider]

Rebased and updated to work with new wallets directory from #11466

[maflcko]

Needs rebase, but I assume this won't work after #11687 anyway?

[laanwj]

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)

[meshcollider]

I've rebased. If #11687 defaults to creating directories for each wallet rather than individual BDB files, then this may need to be modified to list directories as well as BDB files? But yes should still work

[jnewbery]

Tested ACK 946b2ce. One suggested change for the new test.

[jnewbery]

I like this new test, but I think it can be improved in a couple of ways:

• test that a non-empty file without the magic bytes is excluded
• there's an implicit assumption that the symlink and folder created earlier in the test are still around. You could explicitly assert that. If the earlier parts of this test get change, eg the file names are changed, then this check will continue to succeed but won't be testing anything.

What do you think about:

--- a/test/functional/multiwallet.py
+++ b/test/functional/multiwallet.py
@@ -65,8 +65,20 @@ class MultiWalletTest(BitcoinTestFramework):
         self.start_node(0, self.extra_args[0])
 
         # ensure listwallets only names BDB files, not symlinks, other files or directories
-        open(os.path.join(wallet_dir, 'w01.dat'), 'a').close()
-        assert_equal(set(self.nodes[0].listwallets()['available']), {"w22"})
+        with open(os.path.join(wallet_dir, 'w01.dat'), 'a') as wallet_file:
+            # Create a file containing a long string
+            print("a" * 2000, file=wallet_file)
+
+        available_wallets = self.nodes[0].listwallets()['available']
+
+        assert os.path.islink(os.path.join(wallet_dir, 'w12'))
+        assert 'w12' not in available_wallets
+
+        assert os.path.isdir(os.path.join(wallet_dir, 'w11'))
+        assert 'w11' not in available_wallets
+
+        assert os.path.isfile(os.path.join(wallet_dir, 'w01.dat'))
+        assert 'w01.dat' not in available_wallets

[TheBlueMatt]

Code looks good, RPC docs could be more descriptive (and are out-of-date after the change).

[TheBlueMatt]

Need to update the docs above. Maybe also note that false-positives may include files from exceedingly old (pre-0.8, I believe) nodes as well as duplicates of wallets which cannot be opened simultaneously.

[meshcollider]

Update the documentation as per @TheBlueMatt suggestion

[ryanofsky]

utACK 3dac78f

[ryanofsky]

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:

1. Backwards compatible
2. Better performance for listwallets. It could continue to just return the loaded wallets from memory instead of having now having to do disk io.
3. More descriptive function name. listwalletdir naming is consistent with recently added walletdir parameter, and tells you what the function actually does.

[ryanofsky]

Would suggest returning an array of objects rather than an array of strings to future proof this. This would allow including other information like sizes, last modification times, whether databases are flushed, or locked by another process. (Latter two would be meaningful after #11687 when wallet files can be opened in their own berkelydb environments instead of all using the same shared environment).

[promag]

Partially agree with @ryanofsky. Still think one array is enough, see #11485 (comment). And listwallets could have an optional parameter to filter available, loaded or both.

[ryanofsky]

Note: to be compatible with #11687, you would drop the continue above and change this line to something like:

std::ifstream file(fs::is_regular_file(*it) ? it->path() : (it->path() / "wallet.dat")).string(), std::ios::binary);

[ryanofsky]

Might be ready for merge. Unclear if "Code looks good" from @TheBlueMatt above is equivalent to a code ack.

[meshcollider]

@ryanofsky sorry I haven't had time to work on your suggested changes above, I'm happy to let this wait until I've had time to address them

[laanwj]

Closing for now w/ up for grabs, let me know when you start working on this and it needs to be reopened.

[jnewbery]

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.

[promag]

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 (review)

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?

[jarolrod]

@fanquake This shouldn't be Up for grabs anymore. The main idea behind this PR was implemented in #14291.