Skip to content

Change dumpwallet to use appropriate data directory #1416

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

Merged

Conversation

jamescowens
Copy link
Member

by default, with the ability to provide an override to
that in the filespec.

This should also fix the functionality in the debug console.

I have tested in the debug console of both Linux and Windows with just

dumpwallet filename and it works - it appropriately put it in the testnet subdirectory under the data directory.

I tested Linux with dumpwallet /home/jco/walletkeys.dat on testnet and it put it in the specified pathspec.

I tested Windows with dumpwallet "C:\users\JCO\Documents\walletkeys.dat" and it put it in the correct directory. You need to use the quotes. Before the functionality was totally broken in the UI debug console on Windows.

by default, with the ability to provide an override to
that in the filespec.

This should also fix the functionality in the debug console.
@jamescowens
Copy link
Member Author

Ah. @denravonska you should also merge this into staging. It will apply equally well there, and we may need it for beacon testing.

@jamescowens
Copy link
Member Author

Note that Bitcoin's version of this in 0.17 has the following help...

RPCHelpMan{"dumpwallet",
"\nDumps all wallet keys in a human-readable format to a server-side file. This does not allow overwriting existing files.\n"
"Imported scripts are included in the dumpfile, but corresponding BIP173 addresses, etc. may not be added automatically by importwallet.\n"
"Note that if your wallet contains keys which are not derived from your HD seed (e.g. imported keys), these are not covered by\n"
"only backing up the seed itself, and must be backed up too (e.g. ensure you back up the whole dumpfile).\n",
{
{"filename", RPCArg::Type::STR, RPCArg::Optional::NO, "The filename with path (either absolute or relative to bitcoind)"},
},
RPCResult{
"{ (json object)\n"
" "filename" : { (string) The filename with full absolute path\n"
"}\n"
},
RPCExamples{
HelpExampleCli("dumpwallet", ""test"")
+ HelpExampleRpc("dumpwallet", ""test"")
},
}.ToString());

Here is a test from the command line for jco@jco-linux just specifiying a filename without a path. Notice that I changed to the home directory to see where it would put the file. (Would it put the file where I called the daemon from (CWD) or the data directory:

/.bitcoin> cd ~
jco@jco-linux:
> ./.bitcoin/bitcoin-cli.sh dumpwallet walletkeys_bitcoin.dat
{
"filename": "/data2/home/jco/.bitcoin/walletkeys_bitcoin.dat"
}

/data2/home/jco/.bitcoin/walletkeys_bitcoin.dat is not where bitcoind is. This is the data directory. bitcoind is actually in the bin subdirectory underneath that. So bitcoin is really defaulting to the data directory. Their documentation is misleading.

@denravonska
Copy link
Member

Me and @jamescowens had a chat about this on Slack after I wanted us to follow the Bitcoin implementation. However, it's not clear what its implementation is relative to. It says bitcoind but it seems to vary across platforms.

utACK on this as it is very likely it will change in Bitcoin before we rebase.

@jamescowens
Copy link
Member Author

Merging this now. The behavior is consistent with what bitcoin-cli is doing on Linux, although other platforms may be different.

In our implementation here, the path if specified can be either absolute or relative to the data directory. It defaults to the data directory (i.e. equivalent to ./filename).

This is good enough until Bitcoin sorts theirs out.

@jamescowens jamescowens merged commit 955ca11 into gridcoin-community:development Mar 10, 2019
denravonska added a commit that referenced this pull request May 10, 2019
Added:
 - Replace NeuralNetwork with portable C++ scraper #1387 (@jamescowens,
   @tomasbrod, @Cycy, @TheCharlatan, @denravonska).
 - Allow compile flags to be used for depends #1423 (@G-UK).
 - Add stake splitting and side staking info to getmininginfo #1424
   (@jamescowens).
 - Add freedesktop.org desktop file and icon set #1438 (@a123b).

Changed:
 - Disable Qt for windows Travis builds #1276 (@TheCharlatan).
 - Replace use of AppCache PROJECT section with strongly-typed structures #1415
   (@cyrossignol).
 - Change dumpwallet to use appropriate data directory #1416 (@jamescowens).
 - Optimize ExtractXML() calls by avoiding unnecessary string copies #1419
   (@cyrossignol).
 - Change signature of IsLockTimeWithinMinutes #1422 (@jamescowens).
 - Restore old poll output for getmininginfo RPC #1437 (@a123b).
 - Prevent segfault when using rpc savescraperfilemanifest #1439 (@jamescowens).
 - Improve miner status messages for ineligible staking balances #1447
   (@cyrossignol).
 - Enhance scraper log archiving #1449 (@jamescowens).

Fixed:
 - Re-enable full GUI 32-bit Windows builds - part of #1387 (@jamescowens).
 - Re-activate Windows Installer #1409 (@TheCharlatan).
 - Fix Depends and Travis build issues for ARM #1417 (@jamescowens).
 - Fix syncupdate icons #1421 (@jamescowens).
 - Fix potential BOINC crash when reading projects #1426 (@cyrossignol).
 - Fix freeze when unlocking wallet #1428 (@denravonska).
 - Fix RPC after high priority alert #1432 (@denravonska).
 - Fix missing poll in GUI when most recent poll expired #1455 (@cyrossignol).

Removed:
 - Remove old, rudimentary side staking implementation #1381 (@denravonska).
 - Remove auto unlock #1402 (@denravonska).
 - Remove superblock forwarding #1430 (@denravonska).
@jamescowens jamescowens deleted the fixdumpwallet branch October 23, 2019 00:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants