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

Add friendly output to dumpwallet #9740

Merged
merged 2 commits into from Jun 5, 2017

Conversation

Projects
None yet
8 participants
@aideca
Contributor

aideca commented Feb 11, 2017

Add friendly output to dumpwallet #9564.

  • Add help message.
  • Show created dumpfile path.
  • Add rpc test.

@laanwj laanwj added the RPC/REST/ZMQ label Feb 11, 2017

Show outdated Hide outdated src/wallet/rpcdump.cpp
@@ -572,7 +576,9 @@ UniValue dumpwallet(const JSONRPCRequest& request)
EnsureWalletIsUnlocked();
ofstream file;
file.open(request.params[0].get_str().c_str());
boost::filesystem::path filepath = request.params[0].get_str();
filepath = absolute(filepath);

This comment has been minimized.

@laanwj

laanwj Feb 11, 2017

Member

Interesting. Shouldn't it be only accepting absolute paths in the first place?
Otherwise it will depend on the directory that bitcoind happens to have been started in, that'd be pretty crappy API design.

@laanwj

laanwj Feb 11, 2017

Member

Interesting. Shouldn't it be only accepting absolute paths in the first place?
Otherwise it will depend on the directory that bitcoind happens to have been started in, that'd be pretty crappy API design.

This comment has been minimized.

@aideca

aideca Feb 13, 2017

Contributor

Thanx.
IMO, it is better to inform users that path argument is from the viewpoint of bitcoind either absolute nor relative.
The problem is that users expects path argument is the viewpoint of RPC-caller, but actually the one of bitcoind. The former is not same as the latter even if absolute path if they executed on different docker.
So i added doc and output at this time.

@aideca

aideca Feb 13, 2017

Contributor

Thanx.
IMO, it is better to inform users that path argument is from the viewpoint of bitcoind either absolute nor relative.
The problem is that users expects path argument is the viewpoint of RPC-caller, but actually the one of bitcoind. The former is not same as the latter even if absolute path if they executed on different docker.
So i added doc and output at this time.

This comment has been minimized.

@jonasschnelli

jonasschnelli Feb 13, 2017

Member

Maybe an absolute path or if not, relative to the data-dir? If possible?

@jonasschnelli

jonasschnelli Feb 13, 2017

Member

Maybe an absolute path or if not, relative to the data-dir? If possible?

This comment has been minimized.

@JeremyRubin

JeremyRubin Feb 14, 2017

Contributor

Also is there a "namespaced" name for this function? E.g., boost::filesystem::absolute? It'd be good to use the fully qualified name, otherwise it's difficult to hunt down such dependencies/lookup documentation.

I don't have a firm opinion to add to the above conversation though, except that I would add a way to get dumpwallet to serialize results across network (although dangerous if unencrypted/authenticated link) in case the user wants the wallet dump locally (RPC may even be called on a different machine let alone different directory as @aideca points out).

@JeremyRubin

JeremyRubin Feb 14, 2017

Contributor

Also is there a "namespaced" name for this function? E.g., boost::filesystem::absolute? It'd be good to use the fully qualified name, otherwise it's difficult to hunt down such dependencies/lookup documentation.

I don't have a firm opinion to add to the above conversation though, except that I would add a way to get dumpwallet to serialize results across network (although dangerous if unencrypted/authenticated link) in case the user wants the wallet dump locally (RPC may even be called on a different machine let alone different directory as @aideca points out).

This comment has been minimized.

@laanwj

laanwj Feb 14, 2017

Member

Maybe an absolute path or if not, relative to the data-dir? If possible?

Good idea. It's our (effective) convention for command line arguments as well. I think it would be good to chdir the daemon to the data directory at start. This also avoids issues with the daemon holding on to resources (e.g. the starting directory could be deleted later) discussed in #8278.

RPC may even be called on a different machine

A way to dump the wallet over the network could be useful. The practical problem is that there is currently no way to do streaming writes to the network from RPC code, so the entire wallet would have to be dumped in memory, which would result in huge memory and parsing/generating overhead. I looked at streaming to the HTTP in a different context here: #7759.

Also yes there's a bit of a security concern though it's no different from dumpprivkey and friends...

@laanwj

laanwj Feb 14, 2017

Member

Maybe an absolute path or if not, relative to the data-dir? If possible?

Good idea. It's our (effective) convention for command line arguments as well. I think it would be good to chdir the daemon to the data directory at start. This also avoids issues with the daemon holding on to resources (e.g. the starting directory could be deleted later) discussed in #8278.

RPC may even be called on a different machine

A way to dump the wallet over the network could be useful. The practical problem is that there is currently no way to do streaming writes to the network from RPC code, so the entire wallet would have to be dumped in memory, which would result in huge memory and parsing/generating overhead. I looked at streaming to the HTTP in a different context here: #7759.

Also yes there's a bit of a security concern though it's no different from dumpprivkey and friends...

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
Member

MarcoFalke commented Feb 11, 2017

utACK 48abaea

@JeremyRubin

This comment has been minimized.

Show comment
Hide comment
@JeremyRubin

JeremyRubin Feb 14, 2017

Contributor

utACK 48abaea.

I also want to explicitly approve of the returning of a JSON rather than a plain string, it may be useful (in future work?) to augment this call's return value with other data as well (e.g., which user owns the dump file, how many keys were dumped, etc).

Contributor

JeremyRubin commented Feb 14, 2017

utACK 48abaea.

I also want to explicitly approve of the returning of a JSON rather than a plain string, it may be useful (in future work?) to augment this call's return value with other data as well (e.g., which user owns the dump file, how many keys were dumped, etc).

@laanwj laanwj added this to the 0.15.0 milestone Feb 14, 2017

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Feb 14, 2017

Member

This seems ready to be merged once 0.14 is branched off.

Member

laanwj commented Feb 14, 2017

This seems ready to be merged once 0.14 is branched off.

@jnewbery

Definite concept ACK. A couple of nitty comments about language.

I agree that it'd be better to change the default location to be the bitcoin datadir if possible, but that might be out of scope of this small PR.

Show outdated Hide outdated src/wallet/rpcdump.cpp
"1. \"filename\" (string, required) The filename (if not full path, relative to bitcoind path)\n"
"\nResult:\n"
"{ (json object)\n"
" \"dumpfilepath\" : { (string) The filename\n"

This comment has been minimized.

@jnewbery

jnewbery Feb 17, 2017

Member

I'd prefer to use the name "filename" (since this isn't just the path). You could change the help text to say "The filename with full absolute path"

@jnewbery

jnewbery Feb 17, 2017

Member

I'd prefer to use the name "filename" (since this isn't just the path). You could change the help text to say "The filename with full absolute path"

Show outdated Hide outdated src/wallet/rpcdump.cpp
@@ -561,7 +561,11 @@ UniValue dumpwallet(const JSONRPCRequest& request)
"dumpwallet \"filename\"\n"
"\nDumps all wallet keys in a human-readable format.\n"
"\nArguments:\n"
"1. \"filename\" (string, required) The filename\n"
"1. \"filename\" (string, required) The filename (if not full path, relative to bitcoind path)\n"

This comment has been minimized.

@jnewbery

jnewbery Feb 17, 2017

Member

The text is a bit clunky here. Perhaps "The filename with path (either absolute or relative to bitcoind)" is better?

@jnewbery

jnewbery Feb 17, 2017

Member

The text is a bit clunky here. Perhaps "The filename with path (either absolute or relative to bitcoind)" is better?

@aideca

This comment has been minimized.

Show comment
Hide comment
@aideca

aideca Mar 12, 2017

Contributor

Thank you for your comments and sorry for my late reply.

I just updated help text and qualified function name.
"chdir daemon to the data-dir" and "dump the wallet over the network" are quite interesting. But it's out of scope of this PR as jnewbery said.

Contributor

aideca commented Mar 12, 2017

Thank you for your comments and sorry for my late reply.

I just updated help text and qualified function name.
"chdir daemon to the data-dir" and "dump the wallet over the network" are quite interesting. But it's out of scope of this PR as jnewbery said.

@achow101

This comment has been minimized.

Show comment
Hide comment
@achow101

achow101 Apr 10, 2017

Member

Needs rebase.

Member

achow101 commented Apr 10, 2017

Needs rebase.

@aideca

This comment has been minimized.

Show comment
Hide comment
@aideca

aideca Apr 13, 2017

Contributor

Thanx! rebased & fixuped.

Contributor

aideca commented Apr 13, 2017

Thanx! rebased & fixuped.

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Apr 17, 2017

Contributor

ACK 164019d

Contributor

paveljanik commented Apr 17, 2017

ACK 164019d

@laanwj laanwj merged commit 164019d into bitcoin:master Jun 5, 2017

1 check passed

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

@laanwj laanwj referenced this pull request Jun 5, 2017

Closed

TODO for release notes 0.15.0 #9889

12 of 12 tasks complete

laanwj added a commit that referenced this pull request Jun 5, 2017

Merge #9740: Add friendly output to dumpwallet
164019d Add dumpwallet output test (aideca)
9f82134 Add friendly output to dumpwallet refs #9564 (aideca)

Tree-SHA512: 913fcf18d42eebe34173f1f2519973494b1ad2d86d125ff4bf566d6c64aa501c02f8831e6f44812cd87a46916f61c6f510146af406865b31856d8336c173569f

@cdecker cdecker referenced this pull request Sep 3, 2017

Open

dumpwallet behavior #9564

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment