wallet: Add ListWalletDir utility function #14291

How about moving these functions to interfaces/wallet.cpp. I think they don't belong here.

How about moving these functions to interfaces/wallet.cpp. I think they don't belong here.

This is a reasonable place. These methods fit in with the existing getWallets and handleLoadWallet methods. The Node interface is somewhat monolithic and we could consider breaking it up in various ways. But the Wallet interface would be the wrong place to add these methods. Wallet methods operate on individual wallets, not collections of wallets. And these methods need to be available before any Wallet object is constructed.

Should cite the source of this information (https://github.com/file/file/blob/5824af38469ec1ca9ac3ffd251e7afe9dc11e227/magic/Magdir/database#L74-L75) to give credit upstream and make this code easier to understand / debug / update in the future.

Since this is declared as bytes, the memcmp below will only look for little-endian btree files, even on big endian platforms. It seems like it would make more sense to look for little endian files on little-endian platforms, and big endian files on big endian platforms. Changing this to uint32_t bdb_magic = 0x00053162 and passing &bdb_magic to memcmp should accomplish this.

There are a number of problems with this implementation:

1. Because this opens files recursively, it will return btree paths not in the top directory which CWallet::Verify will reject: https://github.com/bitcoin/bitcoin/blob/920c090f63f4990bf0f3b3d1a6d3d8a8bcd14ba0/src/wallet/wallet.cpp#L3824-L3829
2. Because is_regular_file will return true for symlinks pointing to files, this will return symlink paths which CWallet::Verify will reject
3. Because recursive_directory_iterator order is unspecified, on systems where it does postorder instead of preorder traversal, this will return <wallet>/wallet.dat instead of <wallet> paths, which CWallet::Verify will reject.
4. Less serious, but this returns "wallet.dat" instead of "" as the path for the top level wallet. This is inconsistent with top level wallet path returned in current listwallets and getwalletinfo rpcs in the default bitcoin configuration.
5. This is less efficient than it needs to be. Instead of only opening files called wallet.dat in subdirectories, it will read logs and other random files which bitcoin would never open as wallets. It also reads wallet.dat files in subdirectories twice and then uses a std::set to filter out the duplicates after they are read, which is unnecessary.

Following alternate implementation is untested, but I think would avoid these problems:

std::vector<fs::path> wallets;
for (auto it = fs::recursive_directory_iterator(wallet_dir); it != end(it); ++it) {
    if (it->status().type() == fs::directory_file && IsBerkeleyBtree(it->path() / "wallet.dat")) {
        // Found a directory which contains wallet.dat btree file, add it as a wallet.
        wallets.emplace_back(it->path());
    } else if (it.level() == 0 && it->symlink_status().type() == fs::regular_file && IsBerkeleyBtree(it->path())) {
        if (it->path() == "wallet.dat") {
            // Found top-level wallet.dat btree file, add top level directory ""
            // as a wallet.
            wallets.emplace_back();
        } else {
            // Found top-level btree file not called wallet.dat. Current bitcoin
            // software will never create these files but will allow them to be
            // opened in a shared database environment for backwards compatibility.
            // Add it to the list of available wallets.
            wallets.emplace_back(it->path());
        }
    }
}
return wallets;

Nit: Add some trivial test for ListWalletDir to make it technically in use.

There is listwaletdir RPC, which has some tests.

Nit: throwNotWalletBuild()?

I find this function name confusing. How about listWallets()? A future version of this function could take a directory argument and the name would still make sense.

I like the pairing between getWalletDir, listWalletDir and -walletdir, but I don't have a strong preference. Let's see what others say.

I think I agree with promag. listWalletDir describes more specifically what the function does than listWallets and it fits in with the other names cited. I don't imagine us adding a gui feature that would call for the gui asking the node to list wallets in a directory other than -walletdir, but even if we did and this method gained an optional argument, listWalletDir would still seem like a sensible name.

Ditto as above, but unfortunately it's too late to rename listwallets to listloadedwallets. Overloading listwallets doesn't seem like a good idea.

What about lswallets, listwalletfiles or findwallets?

Agree in not overloading listwallets. There are more alternatives in #11485 discussion thread.

I don't see a problem with listwalletdir. It describes the thing this RPC does, which is to list the contents of the -walletdir directory. (I do think it would have been clearer if that directory would have been called -walletsdir instead of -walletdir, but surprising to me as a native english speaker, other people seemed to think -walletsdir was clumsy and not grammatical in #12221).

Sort alphabetically?

I think that's something for the UI to handle. It can sort by other attribute, like last modified, transaction count, balance, etc.

That's fine, as long as the RPC interface does it, that's also a User Interface :-)

Should we sort listwallets result too?

That could be a nice improvement. Not necessarily in this PR, but perhaps there could be a common function to render the list of wallets in RPC, to make sure they're consistent.

Should it ignore case? Should have an option to support numeric/date prefixes?

From the cli point of view, you can do stuff like bitcoin-cli -regtest listwalletdir | jq -r '.wallets | map(.name) | join("\n")' | sort ....

Don't most alphabetical sort functions ignore case (unless it's a tie) and put numbers before letters? I'm a fan of jq, but seems overkill for a simple list. It doesn't seem terribly complicated in C++11 even with UTF-8 (1st answer, 3rd example):

std::wstring a[] = {L"Шла", L"Саша", L"по", L"шоссе", L"и", L"сосала", L"сушку"};
std::sort(std::begin(a), std::end(a), std::locale("en_US.utf8"));
std::wstring_convert<std::codecvt_utf8<wchar_t>> cvt;
for(auto& s: a) std::cout << cvt.to_bytes(s) << ' ';

If you don't mind I prefer to follow up that discussion and possible implementation in both RPC. In this PR I'm just following listwallets ordering and for now having the functionality is enough.

In commit "wallet: Add ListWalletDir utility" (90adb2cd6149af1869e624a2060ec8c4a543cf08)

Probably delete this line, it looks leftover from debugging.

Ops

In commit "rpc: Add listwalletdir RPC" (963eb27e8598e2746966ee4056f177ed332f0279)

I would consider avoiding a mistake we've made previously with other api's (listwallets, listaccounts) where we limited extensibility by returning lists of strings instead of list of json objects. Specifically, I think it'd be better to return

[ {"wallet": ""}, {"wallet": "foo"}, {"wallet": "bar"} ]

instead of:

[ "", "foo", "bar" ]

Because there's more metadata we could be returning about these wallets even without opening them, such as last modified times from the filesystem, status strings indicating whether wallets are presently loaded and locked, and path types indicating whether wallet paths refer to directories, symlinks, or legacy btree files.

Absolutely, my initial response was like:

{
    "wallets": [
        { "name": "" }
    ]
}

which permits adding, for instance, walletdir, count, offset..

@ryanofsky does this looks good to you?

Any concerns about the endianness of data as compared to bdb_magic here?

Any concerns about the endianness of data as compared to bdb_magic here?

Could add a comment here, but this checks for databases in the native format. On big endian systems, the magic bytes are 00 05 31 62, and on little endian systems they are 62 31 05 00.

agree with regard to adding comment; at least I too had no idea that BerkeleyDB files generated on big-endian systems are incompatible!

edit: another theory on IRC is that BerkeleyDB creates the file in native endian but accepts both (does byteswaps if needed)! if this is true, you need to check for both orderings here

here's Oracle's magic file for id'ing BDB files; https://docs.oracle.com/cd/E17275_01/html/programmer_reference/magic.txt tl;dr; check both ways around

so if i get this right: in the legacy case where walletdir == datadir, this will scan every file in the data directory recursively, and check it for BDB version magic? this includes LevelDB database files, block files, debug log files, authcookie files, lock files … can see tons of ways in which this can go wrong or this is somehow avoided?

if not, I'd strongly suggest it should just fail in that case—if the user wants to list non-default wallets, let them create a wallets directory

No, it only recursively scans directories to try to open wallet.dat inside and then check magic bytes.

It checks magic bytes on all files in the walletdir (datadir in the legacy case) — not recursively.

It still might be good to prevent this from opening too many files if there happen to be a lot of stray files in the root directory, or a lot of directories in the tree. Maybe add && (++count) before && IsBerkeleyBtree checks, and if (count > 1000) throw std::runtime_error("Can't list wallets, too many files in walletdir") and the end of the loop as a sanity check.

@ryanofsky an alternative solution is to scan only if walletdir != datadir, WDYT?

@ryanofsky an alternative solution is to scan only if walletdir != datadir, WDYT?

That seems like a worse solution. It doesn't provide protection if there's a looping directory structure or a lot of junk files in the wallet directory. It's less user friendly if users with old datadirs have to manually move database files around in order for this one rpc method to work. It also seems like it would be more complicated to test and explain.

I think my suggestion is better because it would just work in the all the normal cases, and provide a straightforward error message in the unusual case laanwj is concerned about.

It doesn't provide protection if there's a looping directory structure

From the docs:

By default, recursive_directory_iterator does not follow directory symlinks

IIUC considering the above looping would't happen.

By default, recursive_directory_iterator does not follow directory symlinks

I know, I was thinking about more about FUSE mounts and such. Anyway, if you think it complicates code too much to deal with unexpected filesystem stuff and provide a nice error message, I don't personally think you need to add any error handling here. I was just trying to suggest a way to deal with laanwj's concern.

But since you asked WDYT, I think special casing datadir=walletdir is a bad user experience and confusing and ugly and hard to explain.

@ryanofsky @laanwj added these 2 checks to avoid opening unnecessary files.

I'm not sure what purpose these checks serve. Are they supposed to be an optimization? Is the second check supposed to guard against opening lock files? I would suggest adding a comment here or dropping these.

The first check actually seems like the opposite of an optimization, since file_size/ifstream calls below inherently have to check that the file exists, so it seems like this just checks the same condition twice.

The first check fs::exists(path) was there to prevent exceptions from the 2nd check fs::file_size(path). Fixed and added comment.

switch to fs::ifstream post-#13878

LGTM now

wallet/walletutil.cpp:71:36: error: ‘relative’ is not a member of ‘fs’
                 paths.emplace_back(fs::relative(it->path(), wallet_dir));
                                    ^

See: #14528 (travis: Compile once on xenial)

This is wrong, fs::relative resolves symlinks but we allow symlinks to be specified as wallet names. I guess listwalletdir should list both w7 and w7_symlink but in runtime only one could be loaded (actually it's the same wallet but with different names). @ryanofsky @laanwj should I fix this in #14531 or fix in a separate PR?