rpc: Change importwallet to return additional errors #15093

marcinja commented at 10:44 PM on January 3, 2019: contributor

Addresses issues described in #15019.

importwallet now throws an RPC error if input file is empty, or contains no valid keys/scripts. Logs warnings if header/footer lines are missing from wallet dump file.

This PR changes importwallet to log the number of keys and scripts imported, and logs a warning if the header (# Wallet dump file created by Bitcoin) or footer (# End of dump) of the wallet dump file is missing. A functional test was edited to check for the new RPC error. The RPC error is thrown only when no keys/scripts are imported or skipped. Importing files with only known keys/scripts will not throw an error.

All new error messages indicate to the user that files created by dumpwallet must be used instead.

in src/wallet/rpcdump.cpp:588 in fbc968d6fa outdated

 577 | @@ -575,15 +578,33 @@ UniValue importwallet(const JSONRPCRequest& request)
 578 |          int64_t nFilesize = std::max((int64_t)1, (int64_t)file.tellg());
 579 |          file.seekg(0, file.beg);
 580 |  
 581 | +        static const std::string HEADER_STRING = "# Wallet dump created by Bitcoin";

MarcoFalke commented at 10:49 PM on January 3, 2019:

nit: Should use PACKAGE_NAME instead of "Bitcoin"?

marcinja commented at 3:43 PM on January 4, 2019:

PACKAGE_NAME is "Bitcoin Core[default wallet]", but dumpwallet uses "Bitcoin %s" CLIENT_BUILD. I assume you can use importwallet on wallets dumped using an earlier build right?

MarcoFalke commented at 4:11 PM on January 4, 2019:

Oh sure that should work. I see it is only a log-print and not a error, so should be fine to keep this as is.

in src/wallet/rpcdump.cpp:587 in fbc968d6fa outdated

 582 | +        if (file.good()) {
 583 | +            std::string line;
 584 | +            std::getline(file, line);
 585 | +
 586 | +            // Check that the first line has HEADER_STRING as a prefix.
 587 | +            if (line.compare(0, HEADER_STRING.size(), HEADER_STRING) != 0)

MarcoFalke commented at 10:50 PM on January 3, 2019:

nit: Multiline ifs should come with the { and }

MarcoFalke commented at 10:50 PM on January 3, 2019: member

Didn't look closely, just nits (feel free to ignore)

fanquake added the label Wallet on Jan 3, 2019

fanquake added the label RPC/REST/ZMQ on Jan 3, 2019

marcinja commented at 3:03 PM on January 4, 2019: contributor

That's a good point. Changing it to only abort if no keys/scripts are imported and none are skipped. This will also avoid throwing an error if you import the same valid file twice or more.

marcinja force-pushed on Jan 4, 2019

in src/wallet/rpcdump.cpp:606 in ec0dc59c88 outdated

 602 | +            if (line.empty())
 603 | +                continue;
 604 | +
 605 | +            static const std::string FOOTER_STRING = "# End of dump";
 606 | +            if (line[0] == '#') {
 607 | +                if ((line.compare(0, FOOTER_STRING.size(), FOOTER_STRING) == 0)){

practicalswift commented at 9:50 AM on January 5, 2019:

Nit: Drop redundant parentheses here and introduce space before { :-)

marcinja force-pushed on Jan 6, 2019

whearismybitcoins approved

in src/wallet/rpcdump.cpp:688 in bfc2d88071 outdated

 679 | @@ -650,6 +680,16 @@ UniValue importwallet(const JSONRPCRequest& request)
 680 |      RescanWallet(*pwallet, reserver, nTimeBegin, false /* update */);
 681 |      pwallet->MarkDirty();
 682 |  
 683 | +    if (!has_footer)
 684 | +        pwallet->WalletLogPrintf("Wallet file missing footer line, possibly missing some keys/scripts. Make sure input file was created using dumpwallet.\n");
 685 | +
 686 | +    pwallet->WalletLogPrintf("%d keys and %d scripts imported. %d keys and %d scripts skipped\n", num_keys_imported, num_scripts_imported, num_keys_skipped, num_scripts_skipped);
 687 | +
 688 | +    if ((num_keys_imported == 0) && (num_scripts_imported == 0)

promag commented at 11:09 PM on January 8, 2019:

nit, could drop inner () or could could do if(a + b + c + d == 0) {

Also, could move the above log to the else block.

in src/wallet/rpcdump.cpp:683 in bfc2d88071 outdated

 679 | @@ -650,6 +680,16 @@ UniValue importwallet(const JSONRPCRequest& request)
 680 |      RescanWallet(*pwallet, reserver, nTimeBegin, false /* update */);
 681 |      pwallet->MarkDirty();
 682 |  
 683 | +    if (!has_footer)

promag commented at 11:10 PM on January 8, 2019:

nit, same line or add braces.

in src/wallet/rpcdump.cpp:601 in bfc2d88071 outdated

 597 |          while (file.good()) {
 598 |              uiInterface.ShowProgress("", std::max(1, std::min(99, (int)(((double)file.tellg() / (double)nFilesize) * 100))), false);
 599 |              std::string line;
 600 |              std::getline(file, line);
 601 | -            if (line.empty() || line[0] == '#')
 602 | +            if (line.empty())

promag commented at 11:13 PM on January 8, 2019:

nit, same line

in src/wallet/rpcdump.cpp:606 in bfc2d88071 outdated

 602 | +            if (line.empty())
 603 | +                continue;
 604 | +
 605 | +            static const std::string FOOTER_STRING = "# End of dump";
 606 | +            if (line[0] == '#') {
 607 | +                if (line.compare(0, FOOTER_STRING.size(), FOOTER_STRING) == 0) {

promag commented at 11:15 PM on January 8, 2019:

could be has_footer |= line == "# End of dump";?

in src/wallet/rpcdump.cpp:593 in bfc2d88071 outdated

 579 | @@ -575,15 +580,34 @@ UniValue importwallet(const JSONRPCRequest& request)
 580 |          int64_t nFilesize = std::max((int64_t)1, (int64_t)file.tellg());
 581 |          file.seekg(0, file.beg);
 582 |  
 583 | +        static const std::string HEADER_STRING = "# Wallet dump created by Bitcoin";
 584 | +        if (file.good()) {
 585 | +            std::string line;
 586 | +            std::getline(file, line);
 587 | +
 588 | +            // Check that the first line has HEADER_STRING as a prefix.

promag commented at 11:17 PM on January 8, 2019:

I thought lines starting with # were comments which get ignored.

That said, this is breaking change.

marcinja commented at 3:57 PM on January 9, 2019:

Yes, besides the 2 warnings added in this commit the comment lines are ignored. My reasoning for checking them is that it might give a useful hint to the user if the result of using this RPC isn't what they intended (e.g. fewer keys/scripts imported than intended). Maybe it's not as useful with the log statement that gives the number of key/scripts imported and skipped.

I would say it's more of a bug fix than a breaking change. It seems correct that importwallet /dev/null, or accidentally importing a text file should report an error. Unless you think the error condition (nothing skipped/imported) is too strict to be considered a bug fix.

promag commented at 4:08 PM on January 9, 2019:

You can still check and report everything beside comments or am I missing something?

marcinja commented at 4:15 PM on January 9, 2019:

No, that's right.

promag commented at 11:23 PM on January 8, 2019: member

Concept ACK, I think the error can be more informative. I have some doubts regarding the header/footer handling as I don't think that's valid?

marcinja force-pushed on Jan 9, 2019

laanwj deleted a comment on Jan 9, 2019

in src/wallet/rpcdump.cpp:703 in 08cff22dea outdated

 676 | @@ -650,6 +677,16 @@ UniValue importwallet(const JSONRPCRequest& request)
 677 |      RescanWallet(*pwallet, reserver, nTimeBegin, false /* update */);
 678 |      pwallet->MarkDirty();
 679 |  
 680 | +    if (!has_footer) {
 681 | +        pwallet->WalletLogPrintf("Wallet file missing footer line, possibly missing some keys/scripts. Make sure input file was created using dumpwallet.\n");

sipa commented at 6:26 PM on January 9, 2019:

Do we want this as a JSONRPCError rather than just a log file entry?

promag commented at 7:11 PM on January 9, 2019:

Do we want this at all? Isn't it acceptable to someone generate a file to be imported? At least this was very likely before importmulti.

luke-jr commented at 5:43 AM on April 17, 2019:

It doesn't make sense to error based on a comment line. Maybe the footer line should be un-commented, with tolerance for a comment as backward-compatibility?

meshcollider commented at 12:35 AM on September 11, 2020:

Its not actually an error anyway, just a log output

DrahtBot commented at 3:48 AM on January 26, 2019: member

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

Conflicts

No conflicts as of last run.

DrahtBot added the label Needs rebase on Feb 1, 2019

marcinja force-pushed on Feb 6, 2019

DrahtBot removed the label Needs rebase on Feb 6, 2019

DrahtBot added the label Needs rebase on May 9, 2019

marcinja force-pushed on May 13, 2019

DrahtBot removed the label Needs rebase on May 13, 2019

DrahtBot added the label Needs rebase on Jul 26, 2019

Throw error in importwallet if file empty/invalid

importwallet now throws an RPC error if input file is empty, or contains
no valid keys/scripts. Logs warnings if header/footer lines are missing
from wallet dump file.

e76083b317

marcinja force-pushed on Aug 10, 2019

DrahtBot removed the label Needs rebase on Aug 10, 2019

meshcollider commented at 12:36 AM on September 11, 2020: contributor

Code review ACK e76083b3178a19b5a793f32fb37e0769d314cbc3

DrahtBot added the label Needs rebase on Dec 3, 2021

DrahtBot commented at 8:40 AM on December 3, 2021: member

🐙 This pull request conflicts with the target branch and needs rebase.

<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>

marcinja closed this on Dec 10, 2021

DrahtBot locked this on Dec 10, 2022