Followup to #19324 addressing some comments.
Removes the SalvageWallet function in wallettool and instead directly calls RecoverDatabaseFile as suggested in #19324 (review)
Removes the LogPrintfs and tfm::formats in RecoverDatabaseFile as noted in #19324 (review)
Removes the declarations of VerifyEnvironment and VerifyDatabaseFile that were forgotten in walletdb.h as noted in #19324 (comment)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
22 std::string filename;
23 std::shared_ptr<BerkeleyEnvironment> env = GetWalletEnv(file_path, filename);
24
25 if (!env->Open(true /* retry */)) {
26- tfm::format(std::cerr, "Error initializing wallet database environment %s!", env->Directory());
27+ error = strprintf(_("Error initializing wallet database environment %s!"), env->Directory());
Untranslated.
131+ std::vector<bilingual_str> warnings;
132+ bool ret = RecoverDatabaseFile(path, error, warnings);
133+ if (!ret) {
134+ if (warnings.size() > 0) {
135+ for (const auto warning : warnings) {
136+ tfm::format(std::cerr, "%s\n", warning.original);
style nit in commit “wallettool: …”
A range based for loop does not need to be guarded by a size check, so you can remove the if (warnings.size() > 0)
looked at the diff and couldn’t find any issues ACK 7dcb47bf89729d3959efd18a9d6808175b6b85ca
Though, my test (#19078) still fails with the same error as before (#19079)
When using the salvage command, call RecoverDatabaseFile directly
instead of SalvageWallet. Also removes SalvageWallet as it is no longer
needed.
SalvageWallet was doing an additional verify on the database which would
caause the salvage to sometimes fail. This is not needed.
285@@ -286,7 +286,9 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
286 // LoadToWallet call below creates a new CWalletTx that fill_wtx
287 // callback fills with transaction metadata.
288 auto fill_wtx = [&](CWalletTx& wtx, bool new_tx) {
289- assert(new_tx);
290+ if (!new_tx) {
291+ return false;
In commit “Return false instead of asserting when a loaded tx isn’t new” (21349ec568abd5b25b6ca722aa6a9df3037fa4e8)
This definitely needs a comment. I don’t understand when this condition happens. Does it indicate corruption or is it a thing expected to happen in certain cases? Comment doesn’t need to be involved but should give a clue about what is happening here, since by itself this is enigmatic.
return false would restore the previous behavior instead of crashing.
This condition can happen with corruption. It means that the db has the same tx in multiple records which shouldn’t happen. However this isn’t something we should be asserting for because it isn’t a critical error where money can be lost. It should be recoverable with rescan, else zapwallettxs will fix it.
I’ve added a comment indicating this.
getnewaddress and sendmany
In commit “Return false instead of asserting when a loaded tx isn’t new” (fbe8816f1a95e266b59b39327da4374a0442e279)
The fact that #19078 is hitting this is probably a result of the aggressive salvage
Maybe this change is ultimately the right one, but until we clearly understand what is happening I think it would be better to either drop this commit and follow up in a separate PR, or update the commit to ensure the error is not ignored like:
0--- a/src/wallet/walletdb.cpp
1+++ b/src/wallet/walletdb.cpp
2@@ -256,6 +256,7 @@ public:
3 std::map<std::pair<uint256, CKeyID>, CKey> m_descriptor_keys;
4 std::map<std::pair<uint256, CKeyID>, std::pair<CPubKey, std::vector<unsigned char>>> m_descriptor_crypt_keys;
5 std::map<uint160, CHDChain> m_hd_chains;
6+ bool corrupt = false;
7
8 CWalletScanState() {
9 }
10@@ -289,6 +290,7 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
11 if (!new_tx) {
12 // There's probably some corruption here since the tx we just tried to load was already in the wallet
13 // This error is recoverable with zapwallettxs and is not a major failure
14+ wss.corrupt = true;
15 return false;
16 }
17 ssValue >> wtx;
18@@ -730,7 +732,7 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
19 {
20 // losing keys is considered a catastrophic error, anything else
21 // we assume the user can live with:
22- if (IsKeyType(strType) || strType == DBKeys::DEFAULTKEY) {
23+ if (IsKeyType(strType) || strType == DBKeys::DEFAULTKEY || wss.corrupt) {
24 result = DBErrors::CORRUPT;
25 } else if (strType == DBKeys::FLAGS) {
26 // reading the wallet flags can only fail if unknown flags are present
Code review ACK ba259cf5f9118fb8909dbff2ddef29bff26bc32b not including last commit (see review comment). Only changes since last review are rebasing, removing redundant if, and adding new commit.
23 std::shared_ptr<BerkeleyEnvironment> env = GetWalletEnv(file_path, filename);
24
25 bilingual_str open_err;
26 if (!env->Open(open_err)) {
27- tfm::format(std::cerr, "%s\n", open_err.original);
28+ error = strprintf(Untranslated("Error initializing wallet database environment %s!"), env->Directory());
in commit b27b5f7307:
why is open_error ignored and overwritten?
weak review ACK 21349ec568 📜
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3weak review ACK 21349ec568 📜
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUhrVwwAqvcAfQINATTjkFLBIrq8pOifNArW0AAhU8bJESR0qaPTLBK34fOwDopt
8V2tXquE0yfWB0z50u06NprOTIzFoFr3WOqc00ezt1oIafNorbKUp2BW3Ms29vACu
9/ruIRBjXs5msoDPEIx11eQs0/K2SMXLQ/YV8fyLj+HE3dLx1EMzpCyNhh9vEXUYe
10oEPaBhKTNSGiWgxbFHF0xN9EPgM9eop0s6yCmCviCb808z0z5sFHbc9nTlEDtuoI
11+N7jgcuiWrnUK9UrBtYQ4YZp+7s9Eg1eEz3y5cLhFDOIbKRMBUL9u3bZsQkOXOhb
12bsvk4z1DEZUQ8Pg5Rbb+0z398mCJu8TZMbPtRGV/PDfJVJrN1nYmru5wB5XNf7JM
13bzSATvjhgcRBPDWSYhzHWXt0vxSbA9xjwITmcbCIryJogpWSo1SUbxCzlPaFMHvu
14hWq9HlrpuVxbcAEzyHdUfyLa+6rsTIFSn1Z/8GItTJYHfHx7zaw+FGsz/m+06LWt
15Z3r1uwY4
16=kRMa
17-----END PGP SIGNATURE-----
Timestamp of file with hash 3fdf6b1f1067c03925b9d722351cb047b182e9c7b0e592c72a748edce0c99d97 -
Instead of logging or printing these errors and warnings, return them to
the caller.
VerifyEnvironment and VerifyDatabaseFile were removed, but their
declarations weren't. Remove those.
Code review ACK fbe8816f1a95e266b59b39327da4374a0442e279. First three commits are great, but last commit is not my favorite, and I’d prefer to improve it as suggested or move it to a separate PR to follow up later.
Changes since last review are passing along error string in the recover commit, and adding a comment to last commit.
125@@ -147,7 +126,18 @@ bool ExecuteWalletToolFunc(const std::string& command, const std::string& name)
126 WalletShowInfo(wallet_instance.get());
127 wallet_instance->Flush(true);
128 } else if (command == "salvage") {
129- return SalvageWallet(path);
130+ bilingual_str error;
131+ std::vector<bilingual_str> warnings;
132+ bool ret = RecoverDatabaseFile(path, error, warnings);
133+ if (!ret) {
134+ for (const auto warning : warnings) {
Are there any followups queued for this file, or did you want to fix this now?
0wallet/wallettool.cpp:133:33: warning: loop variable 'warning' of type 'const bilingual_str' creates a copy from type 'const bilingual_str' [-Wrange-loop-analysis]
1 for (const auto warning : warnings) {
2 ^
3wallet/wallettool.cpp:133:22: note: use reference type 'const bilingual_str &' to prevent copying
4 for (const auto warning : warnings) {
5 ^~~~~~~~~~~~~~~~~~~~
61 warning generated.
@ryanofsky Are you going to submit this patch as a pull: #19457 (comment) ?
Sure, created #19793