Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wallet: Add ListWalletDir utility function #14291

Merged
merged 5 commits into from Oct 18, 2018
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -1,4 +1,5 @@
New RPC methods
------------

- `getnodeaddresses` returns peer addresses known to this node. It may be used to connect to nodes over TCP without using the DNS seeds.
- `getnodeaddresses` returns peer addresses known to this node. It may be used to connect to nodes over TCP without using the DNS seeds.
- `listwalletdir` returns a list of wallets in the wallet directory which is configured with `-walletdir` parameter.
@@ -34,6 +34,16 @@ void DummyWalletInit::AddWalletOptions() const

const WalletInitInterface& g_wallet_init_interface = DummyWalletInit();

fs::path GetWalletDir()
{
throw std::logic_error("Wallet function called in non-wallet build.");

This comment has been minimized.

Copy link
@Sjors

Sjors Sep 25, 2018

Member

Nit: throwNotWalletBuild()?

}

std::vector<fs::path> ListWalletDir()
{
throw std::logic_error("Wallet function called in non-wallet build.");
}

std::vector<std::shared_ptr<CWallet>> GetWallets()
{
throw std::logic_error("Wallet function called in non-wallet build.");
@@ -38,6 +38,8 @@
#include <univalue.h>

class CWallet;
fs::path GetWalletDir();
std::vector<fs::path> ListWalletDir();
std::vector<std::shared_ptr<CWallet>> GetWallets();

namespace interfaces {
@@ -218,6 +220,18 @@ class NodeImpl : public Node
LOCK(::cs_main);
return ::pcoinsTip->GetCoin(output, coin);
}
std::string getWalletDir() override
{
return GetWalletDir().string();
}
std::vector<std::string> listWalletDir() override
{
std::vector<std::string> paths;
for (auto& path : ListWalletDir()) {
paths.push_back(path.string());
}
return paths;

This comment has been minimized.

Copy link
@Sjors

Sjors Sep 25, 2018

Member

Sort alphabetically?

This comment has been minimized.

Copy link
@promag

promag Sep 25, 2018

Author Member

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

This comment has been minimized.

Copy link
@Sjors

Sjors Sep 25, 2018

Member

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

This comment has been minimized.

Copy link
@promag

promag Sep 25, 2018

Author Member

Should we sort listwallets result too?

This comment has been minimized.

Copy link
@Sjors

Sjors Sep 25, 2018

Member

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.

This comment has been minimized.

Copy link
@promag

promag Sep 26, 2018

Author Member

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

This comment has been minimized.

Copy link
@Sjors

Sjors Sep 26, 2018

Member

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) << ' ';

This comment has been minimized.

Copy link
@promag

promag Sep 26, 2018

Author Member

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.

}

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Sep 22, 2018

Member

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Sep 22, 2018

Contributor

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.

std::vector<std::unique_ptr<Wallet>> getWallets() override
{
std::vector<std::unique_ptr<Wallet>> wallets;
@@ -173,6 +173,12 @@ class Node
//! Get unspent outputs associated with a transaction.
virtual bool getUnspentOutput(const COutPoint& output, Coin& coin) = 0;

//! Return default wallet directory.
virtual std::string getWalletDir() = 0;

//! Return available wallets in wallet directory.
virtual std::vector<std::string> listWalletDir() = 0;

This comment has been minimized.

Copy link
@Sjors

Sjors Sep 25, 2018

Member

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.

This comment has been minimized.

Copy link
@promag

promag Sep 25, 2018

Author Member

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Sep 26, 2018

Contributor

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.


//! Return interfaces for accessing wallets (if any).
virtual std::vector<std::unique_ptr<Wallet>> getWallets() = 0;

@@ -2450,6 +2450,38 @@ static UniValue getwalletinfo(const JSONRPCRequest& request)
return obj;
}

static UniValue listwalletdir(const JSONRPCRequest& request)

This comment has been minimized.

Copy link
@Sjors

Sjors Sep 25, 2018

Member

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?

This comment has been minimized.

Copy link
@promag

promag Sep 25, 2018

Author Member

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Sep 26, 2018

Contributor

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

{
if (request.fHelp || request.params.size() != 0) {
throw std::runtime_error(
"listwalletdir\n"
"Returns a list of wallets in the wallet directory.\n"
"{\n"
" \"wallets\" : [ (json array of objects)\n"
" {\n"
" \"name\" : \"name\" (string) The wallet name\n"
" }\n"
" ,...\n"
" ]\n"
"}\n"
"\nExamples:\n"
+ HelpExampleCli("listwalletdir", "")
+ HelpExampleRpc("listwalletdir", "")
);
}

UniValue wallets(UniValue::VARR);
for (const auto& path : ListWalletDir()) {
UniValue wallet(UniValue::VOBJ);
wallet.pushKV("name", path.string());
wallets.push_back(wallet);
}

UniValue result(UniValue::VOBJ);
result.pushKV("wallets", wallets);
return result;
}

static UniValue listwallets(const JSONRPCRequest& request)
{
if (request.fHelp || request.params.size() != 0)
@@ -4102,6 +4134,7 @@ static const CRPCCommand commands[] =
{ "wallet", "listsinceblock", &listsinceblock, {"blockhash","target_confirmations","include_watchonly","include_removed"} },
{ "wallet", "listtransactions", &listtransactions, {"dummy","count","skip","include_watchonly"} },
{ "wallet", "listunspent", &listunspent, {"minconf","maxconf","addresses","include_unsafe","query_options"} },
{ "wallet", "listwalletdir", &listwalletdir, {} },
{ "wallet", "listwallets", &listwallets, {} },
{ "wallet", "loadwallet", &loadwallet, {"filename"} },
{ "wallet", "lockunspent", &lockunspent, {"unlock","transactions"} },
@@ -4,6 +4,8 @@

#include <wallet/walletutil.h>

#include <util.h>

fs::path GetWalletDir()
{
fs::path path;
@@ -25,3 +27,51 @@ fs::path GetWalletDir()

return path;
}

static bool IsBerkeleyBtree(const fs::path& path)
{
// A Berkeley DB Btree file has at least 4K.
// This check also prevents opening lock files.
boost::system::error_code ec;
if (fs::file_size(path, ec) < 4096) return false;

fs::ifstream file(path.string(), std::ios::binary);
if (!file.is_open()) return false;

file.seekg(12, std::ios::beg); // Magic bytes start at offset 12
uint32_t data = 0;
file.read((char*) &data, sizeof(data)); // Read 4 bytes of file to compare against magic

This comment has been minimized.

Copy link
@etscrivner

etscrivner Oct 7, 2018

Contributor

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 16, 2018

Contributor

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.

This comment has been minimized.

Copy link
@laanwj

laanwj Oct 18, 2018

Member

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


// Berkeley DB Btree magic bytes, from:
// https://github.com/file/file/blob/5824af38469ec1ca9ac3ffd251e7afe9dc11e227/magic/Magdir/database#L74-L75
// - big endian systems - 00 05 31 62
// - little endian systems - 62 31 05 00
return data == 0x00053162 || data == 0x62310500;

This comment has been minimized.

Copy link
@laanwj

laanwj Oct 18, 2018

Member

LGTM now

}

std::vector<fs::path> ListWalletDir()
{
const fs::path wallet_dir = GetWalletDir();
std::vector<fs::path> paths;

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.
paths.emplace_back(fs::relative(it->path(), wallet_dir));
} else if (it.level() == 0 && it->symlink_status().type() == fs::regular_file && IsBerkeleyBtree(it->path())) {

This comment has been minimized.

Copy link
@laanwj

laanwj Oct 16, 2018

Member

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

This comment has been minimized.

Copy link
@promag

promag Oct 16, 2018

Author Member

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.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 16, 2018

Contributor

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.

This comment has been minimized.

Copy link
@promag

promag Oct 16, 2018

Author Member

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 16, 2018

Contributor

@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.

This comment has been minimized.

Copy link
@promag

promag Oct 16, 2018

Author Member

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.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 16, 2018

Contributor

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.

if (it->path().filename() == "wallet.dat") {
// Found top-level wallet.dat btree file, add top level directory ""
// as a wallet.
paths.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.
paths.emplace_back(fs::relative(it->path(), wallet_dir));

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Oct 20, 2018

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

}
}
}

return paths;
}
@@ -5,10 +5,14 @@
#ifndef BITCOIN_WALLET_WALLETUTIL_H
#define BITCOIN_WALLET_WALLETUTIL_H

#include <chainparamsbase.h>
#include <util.h>
#include <fs.h>

#include <vector>

//! Get the path of the wallet directory.
fs::path GetWalletDir();

//! Get wallets in wallet directory.
std::vector<fs::path> ListWalletDir();

This comment has been minimized.

Copy link
@practicalswift

practicalswift Sep 22, 2018

Member

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

This comment has been minimized.

Copy link
@promag

promag Sep 25, 2018

Author Member

There is listwaletdir RPC, which has some tests.

#endif // BITCOIN_WALLET_WALLETUTIL_H
@@ -38,6 +38,8 @@ def wallet_file(name):
return wallet_dir(name, "wallet.dat")
return wallet_dir(name)

assert_equal(self.nodes[0].listwalletdir(), { 'wallets': [{ 'name': '' }] })

# check wallet.dat is created
self.stop_nodes()
assert_equal(os.path.isfile(wallet_dir('wallet.dat')), True)
@@ -68,6 +70,8 @@ def wallet_file(name):
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(map(lambda w: w['name'], self.nodes[0].listwalletdir()['wallets'])), set(['', 'w3', 'w2', 'sub/w5', 'w7', 'w7', 'w1', 'w8', 'w']))

This comment has been minimized.

Copy link
@promag

promag Oct 20, 2018

Author Member

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?


assert_equal(set(node.listwallets()), set(wallet_names))

# check that all requested wallets were created
@@ -139,6 +143,8 @@ def wallet_file(name):

self.restart_node(0, extra_args)

assert_equal(set(map(lambda w: w['name'], self.nodes[0].listwalletdir()['wallets'])), set(['', 'w3', 'w2', 'sub/w5', 'w7', 'w7', 'w8_copy', 'w1', 'w8', 'w']))

wallets = [wallet(w) for w in wallet_names]
wallet_bad = wallet("bad")

@@ -276,6 +282,8 @@ def wallet_file(name):
assert_equal(self.nodes[0].listwallets(), ['w1'])
assert_equal(w1.getwalletinfo()['walletname'], 'w1')

assert_equal(set(map(lambda w: w['name'], self.nodes[0].listwalletdir()['wallets'])), set(['', 'w3', 'w2', 'sub/w5', 'w7', 'w9', 'w7', 'w8_copy', 'w1', 'w8', 'w']))

# Test backing up and restoring wallets
self.log.info("Test wallet backup")
self.restart_node(0, ['-nowallet'])
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.