Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion doc/release-notes-13152.md
Original file line number Diff line number Diff line change
@@ -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.
10 changes: 10 additions & 0 deletions src/dummywallet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.");
Expand Down
14 changes: 14 additions & 0 deletions src/interfaces/node.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort alphabetically?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we sort listwallets result too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@Sjors Sjors Sep 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
Copy link
Contributor

@ken2812221 ken2812221 Sep 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Expand Down
6 changes: 6 additions & 0 deletions src/interfaces/node.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@promag promag Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Expand Down
33 changes: 33 additions & 0 deletions src/wallet/rpcwallet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2450,6 +2450,38 @@ static UniValue getwalletinfo(const JSONRPCRequest& request)
return obj;
}

static UniValue listwalletdir(const JSONRPCRequest& request)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Expand Down Expand Up @@ -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"} },
Expand Down
50 changes: 50 additions & 0 deletions src/wallet/walletutil.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

#include <wallet/walletutil.h>

#include <util.h>

fs::path GetWalletDir()
{
fs::path path;
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@laanwj laanwj Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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())) {
Copy link
Member

@laanwj laanwj Oct 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@promag promag Oct 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}
8 changes: 6 additions & 2 deletions src/wallet/walletutil.h
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is listwaletdir RPC, which has some tests.

#endif // BITCOIN_WALLET_WALLETUTIL_H
8 changes: 8 additions & 0 deletions test/functional/wallet_multiwallet.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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']))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Expand Down Expand Up @@ -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")

Expand Down Expand Up @@ -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'])
Expand Down