Add friendly output to dumpwallet

aideca commented at 5:47 am on February 11, 2017: contributor

Add friendly output to dumpwallet #9564.

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

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

in src/wallet/rpcdump.cpp: in 48abaeae39 outdated

575@@ -572,7 +576,9 @@ UniValue dumpwallet(const JSONRPCRequest& request)
576     EnsureWalletIsUnlocked();
577 
578     ofstream file;
579-    file.open(request.params[0].get_str().c_str());
580+    boost::filesystem::path filepath = request.params[0].get_str();
581+    filepath = absolute(filepath);

laanwj commented at 7:32 am on February 11, 2017:

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.

aideca commented at 1:30 pm on February 13, 2017:

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.

jonasschnelli commented at 1:33 pm on February 13, 2017:

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

JeremyRubin commented at 12:56 pm on February 14, 2017:

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

laanwj commented at 1:22 pm on February 14, 2017:

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 commented at 11:43 am on February 11, 2017: member

utACK 48abaeae39c0485f864dd67cc4a2fcac977bbdc3

JeremyRubin commented at 12:59 pm on February 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).

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

laanwj commented at 1:23 pm on February 14, 2017: member

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

in src/wallet/rpcdump.cpp: in 48abaeae39 outdated

560@@ -561,7 +561,11 @@ UniValue dumpwallet(const JSONRPCRequest& request)
561             "dumpwallet \"filename\"\n"
562             "\nDumps all wallet keys in a human-readable format.\n"
563             "\nArguments:\n"
564-            "1. \"filename\"    (string, required) The filename\n"
565+            "1. \"filename\"    (string, required) The filename (if not full path, relative to bitcoind path)\n"
566+            "\nResult:\n"
567+            "{                           (json object)\n"
568+            "  \"dumpfilepath\" : {        (string) The filename\n"

jnewbery commented at 7:46 pm on February 17, 2017:

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”

in src/wallet/rpcdump.cpp: in 48abaeae39 outdated

560@@ -561,7 +561,11 @@ UniValue dumpwallet(const JSONRPCRequest& request)
561             "dumpwallet \"filename\"\n"
562             "\nDumps all wallet keys in a human-readable format.\n"
563             "\nArguments:\n"
564-            "1. \"filename\"    (string, required) The filename\n"
565+            "1. \"filename\"    (string, required) The filename (if not full path, relative to bitcoind path)\n"

jnewbery commented at 7:47 pm on February 17, 2017:

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

jnewbery commented at 7:49 pm on February 17, 2017: member

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.

aideca commented at 11:53 am on March 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.