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

Conversation

@promag
Copy link
Member

commented Sep 21, 2018

ListWalletDir returns all available wallets in the current wallet directory.

Based on MeshCollider work in pull #11485.

@promag

This comment has been minimized.

Copy link
Member Author

commented Sep 21, 2018

After #11485 (comment) decided to push this.

Consider the following:

$ find wallets
wallets
wallets/wallet.dat
wallets/db.log
wallets/x1
wallets/x1/wallet.dat
wallets/x1/db.log
wallets/x1/.walletlock
wallets/x2
wallets/x2/wallet.dat
wallets/x2/db.log
wallets/x2/.walletlock
wallets/.walletlock
wallets/w
wallets/w/x3
wallets/w/x3/wallet.dat
wallets/w/x3/db.log
wallets/w/x3/.walletlock

ListWalletDir() gives

wallet.dat
x1
x2
w/x3

@promag promag force-pushed the promag:2018-09-list-wallet-dir branch 2 times, most recently Sep 21, 2018

@fanquake fanquake added the Wallet label Sep 22, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2018

Reviewers, this pull request conflicts with the following ones:
  • #13100 (gui: Add dynamic wallets support by promag)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

paths.push_back(path.string());
}
return paths;
}

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.

src/wallet/walletutil.cpp Outdated

std::vector<fs::path> ListWalletDir()
{
const unsigned char bdb_magic[4] = {0x62, 0x31, 0x05, 0x00}; // Berkeley DB Btree magic bytes

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Sep 22, 2018

Contributor

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.

src/wallet/walletutil.cpp Outdated

std::vector<fs::path> ListWalletDir()
{
const unsigned char bdb_magic[4] = {0x62, 0x31, 0x05, 0x00}; // Berkeley DB Btree magic bytes

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Sep 22, 2018

Contributor

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.

src/wallet/walletutil.cpp Outdated
// avoid duplicates due to the recursive iteration
if (!dbs.insert(db).second) continue;

paths.push_back(fs::relative(path, wallet_dir));

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Sep 22, 2018

Contributor

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:

    bitcoin/src/wallet/wallet.cpp

    Lines 3824 to 3829 in 920c090

    // Do some checking on wallet path. It should be either a:
    //
    // 1. Path where a directory can be created.
    // 2. Path to an existing directory.
    // 3. Path to a symlink to a directory.
    // 4. For backwards compatibility, the name of a data file in -walletdir.
  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;
paths.push_back(path.string());
}
return paths;
}

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.

@promag

This comment has been minimized.

Copy link
Member Author

commented Sep 22, 2018

Thanks @ryanofsky, will update accordingly.

@@ -11,4 +11,7 @@
//! 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.

@laanwj laanwj changed the title wallet: Add ListWalletDir utility wallet: Add ListWalletDir utility function Sep 23, 2018

@promag promag force-pushed the promag:2018-09-list-wallet-dir branch Sep 24, 2018

@promag

This comment has been minimized.

Copy link
Member Author

commented Sep 24, 2018

@ryanofsky updated to used your suggested code with some minor details fixed. The result is still relative paths to -walletdir.

@practicalswift will add test.

@promag promag force-pushed the promag:2018-09-list-wallet-dir branch Sep 24, 2018

@Sjors
Copy link
Member

left a comment

Concept ACK. Tested 20db275 on macOS (needs testing on Windows).

My wallet.dat ended up as "" in the list.

Relying on BDB magic bytes to figure out if something is a wallet seems potentially brittle. Can you add BDB4 and BDB5 wallet payloads to the test suite? In addition the test node could dynamically create a wallet, and then try to find it. That way at least some developer will see that test fail on their machine if they compile with a future BDB version that's somehow different.

@@ -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()?

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.

@@ -2450,6 +2450,32 @@ 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).

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.

@ryanofsky
Copy link
Contributor

left a comment

utACK 20db275b250189cfd2200df8e5535aa92afa594a

src/wallet/walletutil.cpp Outdated
std::vector<fs::path> paths;

for (auto it = fs::recursive_directory_iterator(wallet_dir); it != end(it); ++it) {
std::cout << it->path().string() << '-' << it.level() << std::endl;

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Sep 26, 2018

Contributor

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

Probably delete this line, it looks leftover from debugging.

This comment has been minimized.

Copy link
@promag

promag Sep 26, 2018

Author Member

Ops

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

src/wallet/rpcwallet.cpp Outdated
"Returns a list of wallets in the wallet directory.\n"
"\nResult:\n"
"[ (json array of strings)\n"
" \"walletname\" (string) the wallet name\n"

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Sep 26, 2018

Contributor

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.

This comment has been minimized.

Copy link
@promag

promag Sep 26, 2018

Author Member

Absolutely, my initial response was like:

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

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

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

static UniValue listwalletdir(const JSONRPCRequest& request)

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

src/wallet/walletutil.cpp Outdated
@@ -39,7 +39,7 @@ static bool IsBerkeleyBtree(const fs::path& path)
uint32_t data = 0;
file.read((char*) &data, sizeof(data)); // Read 4 bytes of file to compare against magic

return memcmp(&data, &bdb_magic, sizeof(bdb_magic)) == 0;

This comment has been minimized.

Copy link
@promag

promag Sep 26, 2018

Author Member

@ryanofsky does this looks good to you?

@promag promag force-pushed the promag:2018-09-list-wallet-dir branch Sep 26, 2018

@promag

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2018

Squashed and added release notes.

@promag

This comment has been minimized.

Copy link
Member Author

commented Sep 27, 2018

the test node could dynamically create a wallet, and then try to find it. That way at least some developer will see that test fail on their machine if they compile with a future BDB version that's somehow different.

@Sjors does the test change looks good to you?

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2018

Coverage Change (pull 14291) Reference (master)
Lines -0.0043 % 87.0361 %
Functions -0.0186 % 84.1130 %
Branches -0.0052 % 51.5451 %
@Sjors

This comment has been minimized.

Copy link
Member

commented Sep 28, 2018

@promag it covers the second part of my suggestion yes: the wallets are generated by the test framework (using whatever version of BDB is on the dev machine), and then listed.

Can you add BDB4 and BDB5 wallet payloads to the test suite?

This isn't explicitly tested, but because some/most Travis machines use bdb4, it's covered regardless. We're not officially supporting BDB>4 wallets, so that's not important.

tACK fc40216e1b0a3e0462b4119f6e4380ac368ed828 for macOS, modulo wallet.dat being shown as "" (though that can be fixed later).

@promag

This comment has been minimized.

Copy link
Member Author

commented Sep 28, 2018

modulo wallet.dat being shown as "" (though that can be fixed later).

That's the name of the default wallet, so it doesn't needs fix. Try:

bitcoind -regtest &
bitcoin-cli -regtest unloadwallet ""

tACK fc40216 for macOS

Thanks @Sjors 👍

@Sjors

This comment has been minimized.

Copy link
Member

commented Sep 28, 2018

@promag that's weird, but unrelated to this change. When you do listwallets it shows up as wallet.dat. And can you load it with loadwallet ""?

@promag

This comment has been minimized.

Copy link
Member Author

commented Sep 28, 2018

@Sjors here I have:

bitcoind -regtest
bitcoin-cli -regtest  listwallets
[
  ""
]
@Sjors

This comment has been minimized.

Copy link
Member

commented Sep 28, 2018

Try adding a second wallet to bitcoin.conf, then it shows up as "wallet.dat". But in that case you can unload with unloadwallet "wallet.dat". In case you don't have wallets in bitcoin.conf, there's never a need to load wallet.dat, so then it doesn't matter that it's called "".

@laanwj laanwj added this to Blockers in High-priority for review Oct 4, 2018

@meshcollider
Copy link
Member

left a comment

utACK fc40216


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

@promag promag force-pushed the promag:2018-09-list-wallet-dir branch Oct 16, 2018

src/wallet/walletutil.cpp Outdated
const uint32_t bdb_magic = 0x00053162;

if (!fs::exists(path)) return false;
if (fs::file_size(path) < 1024) return false;

This comment has been minimized.

Copy link
@promag

promag Oct 16, 2018

Author Member

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 16, 2018

Contributor

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.

This comment has been minimized.

Copy link
@promag

promag Oct 16, 2018

Author Member

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

@ryanofsky
Copy link
Contributor

left a comment

utACK 47a229e920187dd90af7bba52cc0bfe4baab735c. Only change since last review is adding more IsBerkeleyBtree checks.

src/wallet/walletutil.cpp Outdated
const uint32_t bdb_magic = 0x00053162;

if (!fs::exists(path)) return false;
if (fs::file_size(path) < 1024) return false;

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 16, 2018

Contributor

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.

@promag promag force-pushed the promag:2018-09-list-wallet-dir branch Oct 16, 2018

@ryanofsky
Copy link
Contributor

left a comment

utACK 743a3a9b20768519a11f54834d27fd7585a0670a. Just some cleanup around the file size check since the last review.

@promag promag force-pushed the promag:2018-09-list-wallet-dir branch 2 times, most recently Oct 17, 2018

@promag

This comment has been minimized.

Copy link
Member Author

commented Oct 17, 2018

Thanks, rebased to remove fixup.

src/wallet/walletutil.cpp Outdated
boost::system::error_code ec;
if (fs::file_size(path, ec) < 4096) return false;

std::ifstream file(path.string(), std::ios::binary);

This comment has been minimized.

Copy link
@laanwj

laanwj Oct 18, 2018

Member

switch to fs::ifstream post-#13878

@laanwj

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

utACK after nits

@promag promag force-pushed the promag:2018-09-list-wallet-dir branch Oct 18, 2018

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

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2018

utACK b791ef12c3605c185432e391e1571853b07a7441. Changes since last review were adding minimum file size check, switching to fs::ifstream, and no longer omitting big-endian btree files. It seems that berkeley supports loading databases with any byte order, with native byte order just being slightly more efficient: https://docs.oracle.com/cd/E17276_01/html/programmer_reference/general_am_conf.html#am_conf_byteorder

@jonasschnelli jonasschnelli removed this from Blockers in High-priority for review Oct 18, 2018

@meshcollider

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

re-utACK after squash

@promag promag force-pushed the promag:2018-09-list-wallet-dir branch to d56a068 Oct 18, 2018

@meshcollider

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

utACK d56a068

Let's merge 🎉

@laanwj laanwj merged commit d56a068 into bitcoin:master Oct 18, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Oct 18, 2018

Merge #14291: wallet: Add ListWalletDir utility function
d56a068 docs: Add release notes for listwalletdir RPC (João Barbosa)
0cb3cad qa: Add tests for listwalletdir RPC (João Barbosa)
cc33773 rpc: Add listwalletdir RPC (João Barbosa)
d1b03b8 interfaces: Add getWalletDir and listWalletDir to Node (João Barbosa)
fc4db35 wallet: Add ListWalletDir utility (João Barbosa)

Pull request description:

  `ListWalletDir` returns all available wallets in the current wallet directory.

  Based on MeshCollider work in pull #11485.

Tree-SHA512: 5843e3dbd1e0449f55bb8ea7c241a536078ff6ffcaad88ce5fcf8963971d48c78600fbc4f44919523b8a92329d5d8a5f567a3e0ccb0270fdd27366e19603a716

@promag promag deleted the promag:2018-09-list-wallet-dir branch Oct 18, 2018

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

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

@kallewoof

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

On Debian Jessie PowerPC (Big endian) system (endianness probably unrelated):

wallet/walletutil.cpp: In function ‘std::vector<boost::filesystem::path> ListWalletDir()’:
wallet/walletutil.cpp:57:78: error: ‘end’ was not declared in this scope
     for (auto it = fs::recursive_directory_iterator(wallet_dir); it != end(it); ++it) {
                                                                              ^
wallet/walletutil.cpp:57:78: note: suggested alternative:
In file included from /usr/include/c++/4.9/bits/basic_string.h:42:0,
                 from /usr/include/c++/4.9/string:52,
                 from ./fs.h:9,
                 from ./wallet/walletutil.h:8,
                 from wallet/walletutil.cpp:5:
/usr/include/c++/4.9/initializer_list:99:5: note:   ‘std::end’
     end(initializer_list<_Tp> __ils) noexcept
     ^
wallet/walletutil.cpp:60:32: error: ‘relative’ is not a member of ‘fs’
             paths.emplace_back(fs::relative(it->path(), wallet_dir));
                                ^
wallet/walletutil.cpp:71:36: error: ‘relative’ is not a member of ‘fs’
                 paths.emplace_back(fs::relative(it->path(), wallet_dir));
                                    ^
Makefile:6919: recipe for target 'wallet/libbitcoin_wallet_a-walletutil.o' failed
make[2]: *** [wallet/libbitcoin_wallet_a-walletutil.o] Error 1
make[2]: Leaving directory '/root/workspace/bitcoin/src'
Makefile:10219: recipe for target 'all-recursive' failed
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory '/root/workspace/bitcoin/src'
Makefile:697: recipe for target 'all-recursive' failed
make: *** [all-recursive] Error 1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.