Adds an offline tool bitcoin-wallet-tool for wallet creation and maintenance.
Currently this tool can create a new wallet file and display information on an existing wallet. It can later be extended to support other common wallet maintenance tasks (eg run the salvage and zapwallettxes maintenance tasks on an existing wallet).
Doing wallet maintenance tasks in an offline tool makes much more sense (and is potentially safer) than having to spin up a full node.
This is currently a work in progress. It is a rebase of @jonasschnelli’s #8745. Please see that PR for history and context. I’ve volunteered to maintain this through to merge. Thanks Jonas for your work in writing and maintaining this branch so far!
It is currently rebased on top of #12490, which removes a circular dependency and allows it to build. Only the Add wallet inspection and modification tool “bitcoin-wallet-tool” commit is relevant here.
#12490 can’t be merged until after the V0.17 branch is forked, so detailed review at this point would be immature. Concept and high-level comments most definitely welcomed.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
Will that tool also support things like creating addresses or dumping privkeys in the future
Potentially, yes. That would be a discussion for a future PR though :)
17+#include <stdio.h>
18+
19+
20+static void SetupWalletToolArgs()
21+{
22+ std::string usage;
7+
8+#include <script/ismine.h>
9+#include <wallet/wallet.h>
10+
11+namespace WalletTool {
12+CWallet* CreateWallet(const std::string name, const fs::path& path);
name should be passed by const reference?
8+#include <script/ismine.h>
9+#include <wallet/wallet.h>
10+
11+namespace WalletTool {
12+CWallet* CreateWallet(const std::string name, const fs::path& path);
13+CWallet* LoadWallet(const std::string name, const fs::path& path);
name should be passed by const reference?
Thanks for the review @practicalswift . I’ve fixed your review comments and rebased on the latest #12490.
Since this PR depends on that one, do you mind reviewing that PR?
15+ BLOCK "StringFileInfo"
16+ BEGIN
17+ BLOCK "040904E4" // U.S. English - multilingual (hex)
18+ BEGIN
19+ VALUE "CompanyName", "Bitcoin"
20+ VALUE "FileDescription", "bitcoin-wallet-tool (CLI Bitcoin wallet editor utility)"
Bitcoin with "PACKAGE_NAME ", since it doesn’t work on other Bitcoin wallets.
69+{
70+ SetupEnvironment();
71+ RandomInit();
72+ try {
73+ if(!WalletAppInit(argc, argv))
74+ return EXIT_FAILURE;
How does this compare to #10102
Very different. 10102 is much more ambitious and is a change to the overall architecture of bitcoind. It has dependency on #10973, so I don’t expect it to be merged any time soon. It also doesn’t currently support running the wallet executable separately as far as I’m aware.
This is a small, self-contained tool for created, inspecting and updating wallet files.
0@@ -0,0 +1,103 @@
1+// Copyright (c) 2016 The Bitcoin Core developers
124+ fprintf(stderr, "Error loading %s: Wallet corrupted", name.c_str());
125+ return false;
126+ }
127+ fprintf(stdout, "Zaptxs successful executed. Please rescan your wallet.\n");
128+ } else {
129+ fprintf(stderr, "Unknown command\n");
Are you planning adding tests? In this PR or other?
Yes, I plan to add tests to this PR.
40+ if (!gArgs.ParseParameters(argc, argv, error)) {
41+ fprintf(stderr, "Error parsing command line arguments: %s\n", error.c_str());
42+ return EXIT_FAILURE;
43+ }
44+ if (argc < 2 || HelpRequested(gArgs)) {
45+ std::string usage = strprintf(_("%s wallet-tool version"), PACKAGE_NAME) + " " + FormatFullVersion() + "\n" +
0bitcoin-wallet-tool.cpp:46:29: warning: variable 'usage' is uninitialized when used within its own initialization [-Wuninitialized]
1 usage += "\n" + _("Usage:") + "\n" +
2 ^~~~~
Should be ; instead of + at the end of the line?
97+ ECCVerifyHandle globalVerifyHandle;
98+ ECC_Start();
99+ if (!WalletTool::executeWalletToolFunc(method, name))
100+ return EXIT_FAILURE;
101+ ECC_Stop();
102+ return true;
0bitcoin-wallet-tool.cpp:102:5: warning: bool literal returned from 'main' [-Wmain]
1 return true;
2 ^ ~~~~
12+
13+static CWallet* CreateWallet(const std::string& name, const fs::path& path)
14+{
15+ if (fs::exists(path)) {
16+ fprintf(stderr, "Error: File exists already\n");
17+ return NULL;
0src/wallet/wallettool.cpp:17:16: warning: use nullptr [modernize-use-nullptr]
Please replace NULL with nullptr throughout :-)
76+ LOCK(wallet_instance->cs_wallet);
77+
78+ fprintf(stdout, "Wallet info\n===========\n");
79+ fprintf(stdout, "Encrypted: %s\n", wallet_instance->IsCrypted() ? "yes" : "no");
80+ fprintf(stdout, "HD (hd seed available): %s\n", wallet_instance->GetHDChain().seed_id.IsNull() ? "no" : "yes");
81+ fprintf(stdout, "Keypool Size: %lu\n", (unsigned long)wallet_instance->GetKeyPoolSize());
Segfault with invalid parameter:
0bitcoin-wallet-tool -foo
1Error parsing command line arguments: Invalid parameter -foo
2[1] 16474 segmentation fault ./src/bitcoin-wallet-tool -foo
86+bool executeWalletToolFunc(const std::string& method, const std::string& name)
87+{
88+ fs::path path = fs::absolute(name, GetWalletDir());
89+
90+ if (method == "create") {
91+ CWallet* wallet_instance = CreateWallet(name, path);
wallet_instance leaks?
92+ if (wallet_instance) {
93+ WalletShowInfo(wallet_instance);
94+ wallet_instance->Flush();
95+ }
96+ } else if (method == "info") {
97+ CWallet* wallet_instance = LoadWallet(name, path);
wallet_instance leaks?
540+ $(LIBLEVELDB) \
541+ $(LIBLEVELDB_SSE42) \
542+ $(LIBMEMENV) \
543+ $(LIBSECP256K1)
544+
545+bitcoin_wallet_tool_LDADD += $(BOOST_LIBS) $(BDB_LIBS) $(SSL_LIBS) $(CRYPTO_LIBS) $(MINIUPNPC_LIBS) $(EVENT_PTHREADS_LIBS) $(EVENT_LIBS) $(ZMQ_LIBS)
SSL_LIBS. If the tool wanted to upload the private keys, might as well do it without TLS
$(MINIUPNPC_LIBS) $(EVENT_PTHREADS_LIBS) $(EVENT_LIBS) $(ZMQ_LIBS)?
0➜ bitcoin git:(wallet_tool) ✗ ./src/bitcoin-wallet-tool -name=/tmp/w1 create
1Topping up keypool...
2Wallet info
3===========
4Encrypted: no
5HD (hd seed available): yes
6Keypool Size: 2000
7Transactions: 0
8Address Book: 0
9➜ bitcoin git:(wallet_tool) ✗ ./src/bitcoin-wallet-tool -name=/tmp/w1 create
10Error: File exists already
11➜ bitcoin git:(wallet_tool) ✗ ./src/bitcoin-wallet-tool -name=/tmp/w1/foo/.. create
12Topping up keypool...
13Wallet info
14===========
15Encrypted: no
16HD (hd seed available): yes
17Keypool Size: 2000
18Transactions: 0
19Address Book: 0
20➜ bitcoin git:(wallet_tool) ✗ ls /tmp/w1
21database db.log foo wallet.dat
the 3rd run should fail right?
91+ fprintf(stdout, "Keypool Size: %u\n", wallet_instance->GetKeyPoolSize());
92+ fprintf(stdout, "Transactions: %zu\n", wallet_instance->mapWallet.size());
93+ fprintf(stdout, "Address Book: %zu\n", wallet_instance->mapAddressBook.size());
94+}
95+
96+bool executeWalletToolFunc(const std::string& method, const std::string& name)
134+ fprintf(stderr, "Error loading %s: Wallet corrupted", name.c_str());
135+ return false;
136+ }
137+ fprintf(stdout, "Zaptxs successful executed. Please rescan your wallet.\n");
138+ } else {
139+ fprintf(stderr, "Unknown command\n");
nit
0fprintf(stderr, "Invalid command: %s\n", method.c_str());
18+ wallet->WalletLogPrintf("Releasing wallet\n");
19+ wallet->Flush();
20+ delete wallet;
21+}
22+
23+static std::shared_ptr<CWallet> CreateWallet(const std::string& name, const fs::path& path)
std::unique_ptr?
11+namespace WalletTool {
12+
13+std::shared_ptr<CWallet> CreateWallet(const std::string& name, const fs::path& path);
14+std::shared_ptr<CWallet> LoadWallet(const std::string& name, const fs::path& path);
15+void WalletShowInfo(CWallet* wallet_instance);
16+bool executeWalletToolFunc(const std::string& method, const std::string& file);
bool ExecuteWalletToolFunc....
While running
0./src/bitcoind -wallet=/tmp/w1 -regtest
This happens:
0./src/bitcoin-wallet-tool -name=/tmp/w1 info
1libc++abi.dylib: terminating with uncaught exception of type std::runtime_error: BerkeleyBatch: Failed to open database environment.
2[1] 78930 abort ./src/bitcoin-wallet-tool -name=/tmp/w1 info
534+ $(LIBBITCOIN_COMMON) \
535+ $(LIBBITCOIN_CONSENSUS) \
536+ $(LIBBITCOIN_UTIL) \
537+ $(LIBBITCOIN_CRYPTO) \
538+ $(LIBUNIVALUE) \
539+ $(LIBBITCOIN_ZMQ) \
Could also remove these:
0diff --git a/src/Makefile.am b/src/Makefile.am
1index 41cd066fa0..fc398a5fd9 100644
2--- a/src/Makefile.am
3+++ b/src/Makefile.am
4@@ -539,8 +539,6 @@ bitcoin_wallet_tool_LDADD = \
5 $(LIBBITCOIN_CONSENSUS) \
6 $(LIBBITCOIN_UTIL) \
7 $(LIBBITCOIN_CRYPTO) \
8- $(LIBUNIVALUE) \
9- $(LIBBITCOIN_ZMQ) \
10 $(LIBLEVELDB) \
11 $(LIBLEVELDB_SSE42) \
12 $(LIBMEMENV) \
540+ $(LIBLEVELDB) \
541+ $(LIBLEVELDB_SSE42) \
542+ $(LIBMEMENV) \
543+ $(LIBSECP256K1)
544+
545+bitcoin_wallet_tool_LDADD += $(BOOST_LIBS) $(BDB_LIBS) $(CRYPTO_LIBS)
$(MINIUPNPC_LIBS) to pass travis?
@MarcoFalke @promag Thanks for the review. Will address in due course.
I’ll push a commit with the above tests and for the remaining commands.
Awesome. Thanks @promag!
bitcoin-wallet instead of bitcoin-wallet-tool. I seems unusual to give a command an extra suffix like -tool, though I guess bitcoin-cli also has a similarly descriptive suffix.
I think it’d be nice to call the executable bitcoin-wallet instead of bitcoin-wallet-tool
Seems reasonable to me. We already have bitcoin-tx and not bitcoin-tx-tool. I’ll change the name to bitcoin-wallet.
While running
[…]
This happens:
[…]
I now catch the exception in LoadWallet in wallettool.cpp
19+const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
20+
21+static void SetupWalletToolArgs()
22+{
23+ gArgs.AddArg("-?", "This help message", false, OptionsCategory::OPTIONS);
24+ gArgs.AddArg("-name=<wallet-name>", "Specify wallet name", false, OptionsCategory::OPTIONS);
bitcoin-wallet should take the exact same -walletdir=<dir> and -wallet=<path> options as bitcoind and bitcoin-gui. I don’t think there are benefits to giving this tool a new way of locating and referring to wallets. It just seems inconsistent and confusing.
There is no -walletdir command for this tool afaik, though that would be useful (no rush).
-wallet indeed seems better than -name, especially if we ever decide to give wallets names. However for this tool I have a light preference for making it a positional argument, e.g. ./bitcoin-wallet create blah seems more intuitive to me than ./bitcoin-wallet -wallet=blah create.
bitcoind to begin with. For instance, with psql (postgres client) you can set the database in multiple ways: env var, -d option or positional argument.
50+ fprintf(stdout, "%s", usage.c_str());
51+ return false;
52+ }
53+
54+ // check for printtoconsole, allow -debug
55+ g_logger->m_print_to_console = gArgs.GetBoolArg("-printtoconsole", gArgs.GetBoolArg("-debug", false));
55+ g_logger->m_print_to_console = gArgs.GetBoolArg("-printtoconsole", gArgs.GetBoolArg("-debug", false));
56+
57+ // Check for -testnet or -regtest parameter (Params() calls are only valid after this clause)
58+ try {
59+ SelectParams(gArgs.GetChainName());
60+ } catch (const std::exception& e) {
81+
82+ while (argc > 1 && IsSwitchChar(argv[1][0])) {
83+ argc--;
84+ argv++;
85+ }
86+ std::vector<std::string> args = std::vector<std::string>(&argv[1], &argv[argc]);
43+ fprintf(stdout, "Topping up keypool...\n");
44+ wallet_instance->TopUpKeyPool();
45+ return wallet_instance;
46+}
47+
48+static std::shared_ptr<CWallet> LoadWallet(const std::string& name, const fs::path& path)
520@@ -509,6 +521,32 @@ bitcoin_tx_LDADD = \
521 bitcoin_tx_LDADD += $(BOOST_LIBS) $(CRYPTO_LIBS)
522 #
523
524+# bitcoin-wallet-tool binary #
Concept ACK.
This tool could also be useful as a migration tool if we ever decide to move away from DBD and deprecate that dependency from bitcoind/qt. Already it could have its own --with-incompatible-bdb configure argument (can wait for a future PR).
@theuni should probably take a look at the build system aspects of this.
Needs testing in Windows
How do I use this with -testnet? Do we want to ignore bitcoin.conf. If so, that needs mentioning in the help text. Longer term it seems better to support that, e.g. once creating new addresses is possible, addresstype should be honored.
I assume fresh wallets trigger a rescan because the tool doesn’t know the block height? If so, maybe warn about that, and add a heighthint argument later.
Tested a bit… I think the help should mention that the created wallet is relative to the Bitcoin data dir (from reading the help, I would have expected that the wallet will be created in the current directory).
I personally would prefer that the tool does not interact with the datadir. (specify path or expect current dir).
Also, how can you create a new wallet in the regtest or testnet subdirectory?
I personally would prefer that the tool does not interact with the datadir.
I think that would be a sensible idea for a low-level wallet debugging tool, but a bad idea for a tool that can replace bitcoind and bitcoin-qt as a way of accessing wallets. I think the least confusing way for bitcoin-wallet to work is for it to use the same configuration bitcoind and bitcoin-qt do, and to open and locate wallets in exactly the same way (with exact same -wallet -walletdir -datadir -regtest -conf etc parameters and defaults).
Lets think about an actual use case. If a user who runs bitcoind or bitcoin-qt normally encounters a problem that requires them to run “salvage” or “zaptxs”, it would be nice if they could just run bitcoin-wallet salvage or bitcoin-wallet zaptxs from the command line directly, without having to cd into weird directories or specify long data paths or learn an new set of command line arguments different from the existing ones.
The bitcoin-wallet tool is also going to be extended in the future. I think it’d be nice if it had dump and load subcommands that could be a safe for backing up and transferring wallets into flat files without the user having to know anything about the internal structure of their data dir. I think it’d be nice if there were a shell subcommand that would drop into an rpcconsole like shell and allow running wallets rpc commands in an offline mode.
0@@ -0,0 +1,101 @@
1+#!/usr/bin/env python3
I think that would be a sensible idea for a low-level wallet debugging tool, but a bad idea for a tool that can replace bitcoind and bitcoin-qt as a way of accessing wallets.
I’m not sure about this but I can follow your thoughts. I think one can also argue that the wallet-tool is a low-level accessing tool for experts.
Lets think about an actual use case. If a user who runs bitcoind or bitcoin-qt normally encounters a problem that requires them to run “salvage” or “zaptxs”.
If you have to salvage or zap transactions, you are in low-level debug mode IMO.
However, I think we should… a) … interpret the provided wallet name as filename (with optional path components) b) … if wallet has not been found, check the default datadir/walletdir if it contains the wallet c) … allow -regtest/-testnet switch d) … describe the path behaviour in the help-text
What you’re suggesting sounds good, as long as it doesn’t mean (part (a) is vague) that bitcoin-wallet should interpret the -wallet=<path> option differently from bitcoind and bitcoin-qt
The salvage and zaptxs subcommands are somewhat low level, but I don’t think a dump command would be so obscure, and I think it would be nice if different various tools understood the same options so for example:
0bitcoin-qt -testnet -wallet=foo
1bitcoin-wallet -testnet -wallet=foo dump > foo.txt
could just work without the without the user having to figure out where their datadir is located, where the testnet directory lives inside datadir, whether the testnet directory contains a “wallets” subdirectory, or is from a legacy install that preceded it, and so on.
What happens if you run this tool on a wallet that’s currently opened by bitcoind?
zapwallettxes is sometimes recommended to deal with stuck transactions. Especially before the GUI had basic RBF support, but might still be useful (though it’s a pretty blunt instrument).
42+ wallet_dir = lambda *p: data_dir('wallets', *p)
43+ data_dir = lambda *p: os.path.join(self.nodes[0].datadir, 'regtest', *p)
44+
45+ # TODO calling info on a wallet already opened results in the segfault:
46+ # `libc++abi.dylib: terminating with uncaught exception of type std::runtime_error: BerkeleyBatch: Failed to open database environment`
47+ # the error must be fixed and tested here
tool_wallet.py
127+ std::string backup_filename;
128+ if (!WalletBatch::Recover(path, (void*)&dummyWallet, WalletBatch::RecoverKeysOnlyFilter, backup_filename)) {
129+ fprintf(stderr, "Salvage failed\n");
130+ return false;
131+ }
132+ //TODO, set wallets best block to genesis to enforce a rescan
// and comment.
65+ return true;
66+}
67+
68+int main(int argc, char* argv[])
69+{
70+ SetupEnvironment();
0@@ -0,0 +1,101 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2018 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+"""Test bitcoint-wallet-tool.
I’ve updated the tool to treat -datadir, -wallet, -regtest and -testnet arguments in the same way that bitcoind treats those arguments. I’ve also added help text to explain where it looks for wallet files:
0wallet-tool is an offline tool for creating and interacting with Bitcoin Core wallet files.
1By default wallet-tool will act on wallets in the default wallet directory in the datadir.
2To change the target wallet, use the -datadir, -wallet and -testnet/-regtest arguments.
I believe all comments so far are now addressed.
bitcoind and bitcoind-wallet), but this seems good as a starting point for a new tool that can be extended later.
45+ return false;
46+ }
47+ if (argc < 2 || HelpRequested(gArgs)) {
48+ std::string usage = strprintf("%s bitcoin-wallet version", PACKAGE_NAME) + " " + FormatFullVersion() + "\n\n" +
49+ "wallet-tool is an offline tool for creating and interacting with Bitcoin Core wallet files.\n" +
50+ "By default wallet-tool will act on wallets in the default wallet directory in the datadir.\n" +
testnet=1 in bitcoin.conf is ignored)
My earlier [comment](
#13926 (review)) about -name vs. -wallet is addressed now. I still think ./bitcoin-wallet create blah is more intuitive than ./bitcoin-wallet -wallet=blah create, but can live with both.
Trying to load a wallet that’s currently opened by bitcoind now nicely returns a Error loading . Is wallet being used by other process?
Lightly tested ebd8417 on macOS 10.14. If you call bitcoin-wallet -testnet without arguments it crashes with Segmentation fault: 11 instead of showing the help.
1117@@ -1111,7 +1118,7 @@ else
1118 AC_CHECK_HEADER([openssl/ssl.h],, AC_MSG_ERROR(libssl headers missing),)
1119 AC_CHECK_LIB([ssl], [main],SSL_LIBS=-lssl, AC_MSG_ERROR(libssl missing))
1120
1121- if test x$build_bitcoin_cli$build_bitcoin_tx$build_bitcoind$bitcoin_enable_qt$use_tests != xnonononono; then
1122+ if test x$build_bitcoin_wallet$build_bitcoin_cli$build_bitcoin_tx$build_bitcoind$bitcoin_enable_qt$use_tests != xnononononono; then
51+ fprintf(stderr, "Error: Wallet files does not exist\n");
52+ return nullptr;
53+ }
54+
55+ std::shared_ptr<CWallet> wallet_instance(new CWallet(name, WalletDatabase::Create(path)), WalletToolReleaseWallet);
56+ bool first_run;
first_run :-)
I think this will be a really useful tool to add more functionality too that isn’t really suited to adding to bitcoind itself. Just testing this (have done some light code review, didn’t look at the build_msvc/ files or configure.ac & Makefile.am changes). A few early comments:
Some of @ryanofsky’s comments mentioned above still need addressing, e.g. that the -printtoconsole and -debug args still aren’t recognized and error if they are tried to be used. I am slightly worried about the code duplication too but I think its ok.
Calling info or zaptxs on a non-existent wallet should throw and error, but instead it creates a file and prints info of a wallet containing no keys, transactions, etc. Then calling create on the same wallet name will error that the file already exists.
One other note, the tool should check a command was provided. Currently it just exits without doing anything if no command is provided.
111+ wallet_instance->Flush();
112+ }
113+ } else if (command == "info") {
114+ std::string error;
115+ if (!WalletBatch::VerifyEnvironment(path, error)) {
116+ fprintf(stderr, "Error loading %s. Is wallet being used by other process?\n", name.c_str());
master :-)
37+}
38+
39+static bool WalletAppInit(int argc, char* argv[])
40+{
41+ SetupWalletToolArgs();
42+ std::string error;
errorMessage instead to avoid shadowing error from src/util/system.h?
Addressing @MeshCollider’s feedback:
Some of @ryanofsky’s comments mentioned above still need addressing, e.g. that the -printtoconsole and -debug args still aren’t recognized and error if they are tried to be used. I am slightly worried about the code duplication too but I think its ok.
These should all be addressed now.
Calling info or zaptxs on a non-existent wallet should throw and error, but instead it creates a file and prints info of a wallet containing no keys, transactions, etc. Then calling create on the same wallet name will error that the file already exists.
This is fixed.
One other note, the tool should check a command was provided. Currently it just exits without doing anything if no command is provided.
This is fixed.
Latest force-push should address all remaining feedback: https://github.com/bitcoin/bitcoin/compare/21b4b35a8ecf879ec5909820ab36b3aa29358fe7..d05ec61f7659466e911ea3ae48aa583c792d8849
Thanks everyone for review! Please let me know if I’ve missed anything.
utACK 7064a9515dd7de8d32a1e4e7c08cb0ce17677b23. Changes since last review: rebase, tweaks like adding better error checking, supporting debug/printtoconsole.
Deduplicating wallet load & create wallet code between bitcoind & bitcoin-wallet would be nice cleanup to follow up with in the future.
I find the following behavior a bit scary:
0$ src/bitcoin-wallet help zaptxs
1Zaptxs successful executed. Please rescan your wallet.
I understand that we don’t want to duplicate the help in this initial version, but it should probably throw an error.
I’m also confused how zaptxs is supposed to work here. In bitcoind -zapwallettxes takes an argument and actually leads to a rescan. When I call bitcoin-wallet zaptxs and then open the zapped wallet the transactions and labels are still there.
I’m not sure how to test salvage.
If bitcoin.conf sets testnet=1 there’s no way to acces a mainnet wallet (not a big deal).
The wallet creation command doesn’t take arguments (like watch-only). I suppose that’s fine for this initial version.
I think it would be good to merge wallet tool and make a new issue or set of issues to keep track of the usability problems and feature deficits Sjors is bringing up.
I don’t think we should be trying to perfect this tool in a single github PR. I think it would be better to merge it, mark it as experimental, and continue to develop it in git. This would make any proposed changes easier to understand and discuss.
The wallet creation command doesn’t take arguments (like watch-only). I suppose that’s fine for this initial version.
One idea for this might be to remove most functionality from the wallet tool, and just allow it to fill JSONRPCRequest objects and call the limited subset of RPC methods that work offline. This would also let it share named/positional argument parsing code with bitcoin-cli.
Thanks for the input @Sjors and @ryanofsky .
I find the following behavior a bit scary: …
Good catch! I’ve added that as an error condition (with test) in https://github.com/bitcoin/bitcoin/pull/13926/commits/c904860a24aade9a59f9948eb5e8e3da80a7b12f
I don’t think we should be trying to perfect this tool in a single github PR
Yes. Good point. I’ve removed the salvage and zaptxs commands in this PR, so this tool now can only provide info or create a wallet. I’ll move the salvage and zaptxs commands to a follow-up PR. Done in https://github.com/bitcoin/bitcoin/pull/13926/commits/3519b2f0fb0f8fa22a788471d32917cefad61d82.
@ryanofsky ’s suggestion matches the original intent when the new tools (e.g. bitcoin-tx) were introduced, and plans were made for others (bitcoin-script, bitcoin-key, and bitcoin-wallet):
Once of Linus’s favorite wisdoms was that the Internet is the best test lab in the world, and your goal should be to accelerate changes to the point where they are being tested in the field by real users. Wider audience = more testing and real world feedback.
Small+shipping is a good risk-adjusting model that maximizes the amount of field testing and review. And the common pattern in OSS of endlessly nitpicking small details winds up being counter-productive to higher code quality (by restricting testing to The People Who Follow This Unmerged PR).
Getting software into the field also validates basic developer assumptions of the software’s value, utility and design.
One more thing: src/bitcoin-wallet needs to be added to .gitignore.
Otherwise tACK a92b608
src/bitcoin-wallet needs to be added to .gitignore.
60+ // check for printtoconsole, allow -debug
61+ g_logger->m_print_to_console = gArgs.GetBoolArg("-printtoconsole", gArgs.GetBoolArg("-debug", false));
62+
63+ if (!fs::is_directory(GetDataDir(false))) {
64+ fprintf(stderr, "Error: Specified data directory \"%s\" does not exist.\n", gArgs.GetArg("-datadir", "").c_str());
65+ return EXIT_FAILURE;
We are not in main here, which has a return type int. The function signature here is static bool WalletAppInit, so the return type is bool. Commonly EXIT_FAILURE is 1, so bool(1) == true, which is off-by-one.
You can also test this trivially by providing a datadir that does not exist. It will print an error, but continue and print garbage.
This commit adds wallet-tool, a tool for creating and interacting with
wallet files. Original implementation was by Jonas Schnelli
<dev@jonasschnelli.ch> with modifications by John Newbery
<john@johnnewbery.com>
MSVC files were provided by Chun Kuan Lee <ken2812221@gmail.com>:
build: Add MSVC project files for bitcoin-wallet-tool
Original tests by João Barbosa <joao.paulo.barbosa@gmail.com>
Additional contribution by John Newbery <john@johnnewbery.com>
info and create
15+// queue, which doesn't exist for the bitcoin-wallet. Define our own
16+// deleter here.
17+static void WalletToolReleaseWallet(CWallet* wallet)
18+{
19+ wallet->WalletLogPrintf("Releasing wallet\n");
20+ wallet->Flush();
Flush() calls in this file accept a bool that should be set to true on shutdown?
Yes, you’re right. This got missed out when rebasing on a commit that changed the function signature for Wallet::Flush().
I think the same is true for https://github.com/bitcoin/bitcoin/blob/d8794a78a887a920276c7124f1c46d69592c6c4e/src/wallet/wallet.cpp#L96
EDIT: ignore point about wallet.cpp above
Fixed: #15390
Labels
Wallet
Scripts and tools
Milestone
0.18.0