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

[Tools] bitcoin-wallet - a tool for creating and managing wallets offline #13926

Merged
merged 2 commits into from Jan 31, 2019

Conversation

@jnewbery
Copy link
Member

jnewbery commented Aug 9, 2018

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.

@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Aug 9, 2018

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.

@domob1812

This comment has been minimized.

Copy link
Contributor

domob1812 commented Aug 9, 2018

Concept ACK, looks useful. Will that tool also support things like creating addresses or dumping privkeys in the future, or "just" actual maintenance of the wallet file? That's something that might be useful for offline systems (although you can of course spin up the daemon with an empty data dir).

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Aug 9, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #14372 (msvc: build secp256k1 and leveldb locally by ken2812221)
  • #13787 (Test for Windows encoding issue by ken2812221)

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.

@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Aug 10, 2018

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

@domob1812

This comment has been minimized.

Copy link
Contributor

domob1812 commented Aug 10, 2018

@jnewbery: Yes, of course - let's go in steps. For me, those functions would have been useful from time to time, though, so I'd love to see that added in some follow up. (And am happy to work on it myself if that helps.)

@MeshCollider

This comment has been minimized.

Copy link
Member

MeshCollider commented Aug 14, 2018

Concept ACK, does seem clean and useful to have these as a separate tool.

@DrahtBot DrahtBot referenced this pull request Aug 14, 2018

Closed

[wallet] Kill accounts #13825

@jnewbery jnewbery force-pushed the jnewbery:wallet_tool branch 3 times, most recently Aug 21, 2018

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Aug 22, 2018

Thanks for picking this up, I agree an external wallet manipulation/recovery tool is useful

@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Aug 22, 2018

Thanks all concept ACKers! If you want to help with this PR, please review #12490, since that's a pre-req to get this to build. That PR just removes deprecated code, so it should be an easy review.

@jnewbery jnewbery force-pushed the jnewbery:wallet_tool branch Aug 23, 2018

src/bitcoin-wallet-tool.cpp Outdated

static void SetupWalletToolArgs()
{
std::string usage;

This comment has been minimized.

@practicalswift

practicalswift Sep 5, 2018

Member

Unused variable :-)

This comment has been minimized.

@jnewbery

jnewbery Sep 6, 2018

Author Member

removed

src/wallet/wallettool.h Outdated
#include <wallet/wallet.h>

namespace WalletTool {
CWallet* CreateWallet(const std::string name, const fs::path& path);

This comment has been minimized.

@practicalswift

practicalswift Sep 5, 2018

Member

name should be passed by const reference?

This comment has been minimized.

@jnewbery

jnewbery Sep 6, 2018

Author Member

updated

src/wallet/wallettool.h Outdated

namespace WalletTool {
CWallet* CreateWallet(const std::string name, const fs::path& path);
CWallet* LoadWallet(const std::string name, const fs::path& path);

This comment has been minimized.

@practicalswift

practicalswift Sep 5, 2018

Member

name should be passed by const reference?

This comment has been minimized.

@jnewbery

jnewbery Sep 6, 2018

Author Member

updated

@jnewbery jnewbery force-pushed the jnewbery:wallet_tool branch Sep 6, 2018

@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Sep 6, 2018

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?

@jnewbery jnewbery force-pushed the jnewbery:wallet_tool branch Sep 7, 2018

@jnewbery jnewbery changed the title [WIP] [Tools] bitcoin-wallet-tool [Tools] bitcoin-wallet-tool Sep 7, 2018

@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Sep 7, 2018

#12490 has been merged. I've rebased on master and removed the [WIP] tag.

This is ready for review.

@MarcoFalke
Copy link
Member

MarcoFalke left a comment

haven't looked at the code yet. Just two style nits.

src/bitcoin-wallet-tool-res.rc Outdated
BLOCK "040904E4" // U.S. English - multilingual (hex)
BEGIN
VALUE "CompanyName", "Bitcoin"
VALUE "FileDescription", "bitcoin-wallet-tool (CLI Bitcoin wallet editor utility)"

This comment has been minimized.

@MarcoFalke

MarcoFalke Sep 7, 2018

Member

nit: replace Bitcoin with "PACKAGE_NAME ", since it doesn't work on other Bitcoin wallets.

This comment has been minimized.

@jnewbery

jnewbery Sep 7, 2018

Author Member

agree. Changed

src/bitcoin-wallet-tool.cpp Outdated
RandomInit();
try {
if(!WalletAppInit(argc, argv))
return EXIT_FAILURE;

This comment has been minimized.

@MarcoFalke

MarcoFalke Sep 7, 2018

Member

nit: This indentation is misleading. Should probably use the clang-format-diff script to fix all of these.

This comment has been minimized.

@jnewbery

jnewbery Sep 7, 2018

Author Member

Thanks. I've used clang-format-diff to fix these.

@jnewbery jnewbery force-pushed the jnewbery:wallet_tool branch Sep 7, 2018

@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Sep 7, 2018

Rebased on master and fixed @MarcoFalke's comments.

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Sep 8, 2018

How does this compare to #10102 (@ryanofsky) which also creates an independent wallet executable that could, with some tweaks, be used offline? Obviously this change is much simpler.

@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Sep 10, 2018

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.

@promag
Copy link
Member

promag left a comment

Concept ACK. Are you planning adding tests? In this PR or other?

src/bitcoin-wallet-tool.cpp Outdated
@@ -0,0 +1,103 @@
// Copyright (c) 2016 The Bitcoin Core developers

This comment has been minimized.

@promag

promag Sep 10, 2018

Member

nit, 2018 — 2 more below.

Show resolved Hide resolved src/wallet/wallettool.cpp Outdated
@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Sep 10, 2018

Are you planning adding tests? In this PR or other?

Yes, I plan to add tests to this PR.

@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Jan 17, 2019

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 c904860

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

@jnewbery jnewbery force-pushed the jnewbery:wallet_tool branch Jan 17, 2019

@jnewbery

This comment has been minimized.

@jgarzik

This comment has been minimized.

Copy link
Contributor

jgarzik commented Jan 17, 2019

@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):

  • Start small.
  • Get it in-tree.
  • Discuss and grow from there.

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.

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Jan 17, 2019

One more thing: src/bitcoin-wallet needs to be added to .gitignore.

Otherwise tACK a92b608

@jnewbery jnewbery force-pushed the jnewbery:wallet_tool branch to 2aeac46 Jan 17, 2019

@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Jan 17, 2019

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Jan 17, 2019

re-tACK 2aeac46

@MarcoFalke MarcoFalke added this to the 0.18.0 milestone Jan 17, 2019


if (!fs::is_directory(GetDataDir(false))) {
fprintf(stderr, "Error: Specified data directory \"%s\" does not exist.\n", gArgs.GetArg("-datadir", "").c_str());
return EXIT_FAILURE;

This comment has been minimized.

@MarcoFalke

MarcoFalke Jan 29, 2019

Member

This is not defined behavior, since EXIT_FAILURE has no defined type nor value, but you need a bool(false). c.f. https://en.cppreference.com/w/c/program/EXIT_status

This comment has been minimized.

@jnewbery

jnewbery Jan 29, 2019

Author Member

I don't understand this. There are plenty of other locations where return EXIT_FAILURE is used (and even a couple of PRs where return false has been changed to return EXIT_FAILURE - #9067 and #13349)

This comment has been minimized.

@MarcoFalke

MarcoFalke Jan 29, 2019

Member

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 comment has been minimized.

@jnewbery

jnewbery Jan 29, 2019

Author Member

Duh. Thanks! I only looked at the github snippet. Will fix.

This comment has been minimized.

@jnewbery

jnewbery Jan 30, 2019

Author Member

fixed

jonasschnelli and others added some commits Sep 16, 2016

[tools] Add wallet inspection and modification tool
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
[tests] Add wallet-tool test
Original tests by João Barbosa <joao.paulo.barbosa@gmail.com>

Additional contribution by John Newbery <john@johnnewbery.com>

@jnewbery jnewbery force-pushed the jnewbery:wallet_tool branch from 2aeac46 to 3c3e31c Jan 30, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Jan 30, 2019

Concept ACK. Going to test and merge this this week. Should be safe, since the only methods are info and create

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Jan 31, 2019

Merge bitcoin#13926: [Tools] bitcoin-wallet - a tool for creating and…
… managing wallets offline

3c3e31c [tests] Add wallet-tool test (João Barbosa)
49d2374 [tools] Add wallet inspection and modification tool (Jonas Schnelli)

Pull request description:

  Adds an offline tool `bitcoin-wallet-tool` for wallet creation and maintenance.

  Currently this tool can create a new wallet file, display information on an existing wallet, and run the salvage and zapwallettxes maintenance tasks on an existing wallet. It can later be extended to support other common wallet maintenance tasks.

  Doing wallet maintenance tasks in an offline tool makes much more sense (and is potentially safer) than having to spin up a full node.

Tree-SHA512: 75a28b8a58858d9d76c7532db40eacdefc5714ea5aab536fb1dc9756e2f7d750d69d68d59c50a68e633ce38fb5b8c3e3d4880db30fe01561e07ce58d42bceb2b

@MarcoFalke MarcoFalke merged commit 3c3e31c into bitcoin:master Jan 31, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jnewbery jnewbery deleted the jnewbery:wallet_tool branch Jan 31, 2019

ken2812221 added a commit to ken2812221/bitcoin that referenced this pull request Feb 1, 2019

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Feb 4, 2019

Merge bitcoin#15325: msvc: Fix silent merge conflict between bitcoin#…
…13926 and bitcoin#14372

bef8fdd msvc: Fix silent merge conflict between bitcoin#13926 and bitcoin#14372 (ken2812221)

Pull request description:

  The bitcoin-wallet.exe would have to link with libsecp256k1 after we build libsecp256k1 in project.

Tree-SHA512: cb3fafa301f39121f5d26ac8ac6009c9665fcad1061dbf14ba013104870abe5413ac57c97c97df12b6ba2ad709b776c51aeec20d41f3ae01d3460a5e18f40eec

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Feb 5, 2019

static void WalletToolReleaseWallet(CWallet* wallet)
{
wallet->WalletLogPrintf("Releasing wallet\n");
wallet->Flush();

This comment has been minimized.

@MarcoFalke

MarcoFalke Feb 6, 2019

Member

question: The Flush() calls in this file accept a bool that should be set to true on shutdown?

This comment has been minimized.

@jnewbery

jnewbery Feb 12, 2019

Author Member

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

wallet->Flush();

EDIT: ignore point about wallet.cpp above

Fixed: #15390

ken2812221 added a commit to ken2812221/bitcoin that referenced this pull request Feb 14, 2019

MarcoFalke added a commit that referenced this pull request Feb 14, 2019

Merge #15407: msvc: Fix silent merge conflict between #13926 and #14372
… part II

3c6ef03 msvc: Fix silent merge conflict between #13926 and #14372 part II (Chun Kuan Lee)

Pull request description:

  In #15325, I added secp256k1 as a dependency of bitcoin-wallet. However, I didn't notice that leveldb is also a dependency of it.

Tree-SHA512: dc29b5cad6c529dd9517d6c2cbbe5297b69e73303e2fbbcd4b4842c9c5b51a4332df5a4bf3b82cd3ed2c1668cc95f8c9636f9485af0d722fed9c1319da3cc2e2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment