wallet: Cleanup wallettool salvage and walletdb extraneous declarations #19457

achow101 · 2020-07-06T18:46:19Z

Followup to #19324 addressing some comments.

Removes the SalvageWallet function in wallettool and instead directly calls RecoverDatabaseFile as suggested in #19324 (comment)

Removes the LogPrintfs and tfm::formats in RecoverDatabaseFile as noted in #19324 (comment)

Removes the declarations of VerifyEnvironment and VerifyDatabaseFile that were forgotten in walletdb.h as noted in #19324 (comment)

DrahtBot · 2020-07-06T19:24:32Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Remove wallet.dat path handling from wallet.cpp, rpcwallet.cpp #19619 (Remove wallet.dat path handling from wallet.cpp, rpcwallet.cpp by ryanofsky)
wallettool: Add dump and createfromdump commands #19137 (wallettool: Add dump and createfromdump commands by achow101)
wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets #19077 (wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets by achow101)

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.

src/wallet/salvage.cpp

ryanofsky

Code review ACK 7dcb47bf89729d3959efd18a9d6808175b6b85ca. Nice simple cleanups!

maflcko

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)

src/wallet/wallettool.cpp

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.

achow101 · 2020-07-22T16:35:06Z

Rebased. I've also added a commit that fixes the first error in #19078

ryanofsky

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.

06e263a Call RecoverDatabaseFile directly from wallettool (1/4)
b27b5f73072e1ecbd4de79361fc5609274ed78c2 wallettool: Have RecoverDatabaseFile return errors and warnings (2/4)
ba259cf5f9118fb8909dbff2ddef29bff26bc32b walletdb: Remove unused static functions from walletdb.h (3/4)
21349ec568abd5b25b6ca722aa6a9df3037fa4e8 Return false instead of asserting when a loaded tx isn't new (4/4)

ryanofsky · 2020-07-24T19:17:06Z

src/wallet/walletdb.cpp

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.

The assert has been added in 65b9d8f?w=1 , assuming that adding it does not change behavior. Making it a 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.

This shouldn't be corruption, or at least it seems to happen in normal operation. All that the test in #19078 does is call getnewaddress and sendmany

The fact that #19078 is hitting this is probably a result of the aggressive salvage which, afaik, sometimes creates corruption when there isn't corruption. It's probably from BDB shuffling data around (as part of the normal btree operations) and leaving behind records that are marked as deleted but not actually deleted. So the aggressive salvage still finds these "deleted" records.

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:

--- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -256,6 +256,7 @@ public: std::map<std::pair<uint256, CKeyID>, CKey> m_descriptor_keys; std::map<std::pair<uint256, CKeyID>, std::pair<CPubKey, std::vector<unsigned char>>> m_descriptor_crypt_keys; std::map<uint160, CHDChain> m_hd_chains; + bool corrupt = false; CWalletScanState() { } @@ -289,6 +290,7 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, if (!new_tx) { // There's probably some corruption here since the tx we just tried to load was already in the wallet // This error is recoverable with zapwallettxs and is not a major failure + wss.corrupt = true; return false; } ssValue >> wtx; @@ -730,7 +732,7 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) { // losing keys is considered a catastrophic error, anything else // we assume the user can live with: - if (IsKeyType(strType) || strType == DBKeys::DEFAULTKEY) { + if (IsKeyType(strType) || strType == DBKeys::DEFAULTKEY || wss.corrupt) { result = DBErrors::CORRUPT; } else if (strType == DBKeys::FLAGS) { // reading the wallet flags can only fail if unknown flags are present

maflcko

weak review ACK 21349ec568 📜

Show signature and timestamp

Signature:

Timestamp of file with hash 3fdf6b1f1067c03925b9d722351cb047b182e9c7b0e592c72a748edce0c99d97 -

src/wallet/salvage.cpp

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.

ryanofsky

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.

ryanofsky · 2020-08-12T20:07:15Z

src/wallet/walletdb.cpp

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:

--- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -256,6 +256,7 @@ public: std::map<std::pair<uint256, CKeyID>, CKey> m_descriptor_keys; std::map<std::pair<uint256, CKeyID>, std::pair<CPubKey, std::vector<unsigned char>>> m_descriptor_crypt_keys; std::map<uint160, CHDChain> m_hd_chains; + bool corrupt = false; CWalletScanState() { } @@ -289,6 +290,7 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, if (!new_tx) { // There's probably some corruption here since the tx we just tried to load was already in the wallet // This error is recoverable with zapwallettxs and is not a major failure + wss.corrupt = true; return false; } ssValue >> wtx; @@ -730,7 +732,7 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) { // losing keys is considered a catastrophic error, anything else // we assume the user can live with: - if (IsKeyType(strType) || strType == DBKeys::DEFAULTKEY) { + if (IsKeyType(strType) || strType == DBKeys::DEFAULTKEY || wss.corrupt) { result = DBErrors::CORRUPT; } else if (strType == DBKeys::FLAGS) { // reading the wallet flags can only fail if unknown flags are present

achow101 · 2020-08-13T00:12:53Z

I've dropped the last commit. I agree it doesn't fit well here and can be for a followup.

meshcollider

Code review ACK 0e279fe

ryanofsky

Code review ACK 0e279fe, just dropped last commit

fanquake · 2020-08-14T07:20:23Z

src/wallet/wallettool.cpp

+            std::vector<bilingual_str> warnings;
+            bool ret = RecoverDatabaseFile(path, error, warnings);
+            if (!ret) {
+                for (const auto warning : warnings) {


Are there any followups queued for this file, or did you want to fix this now?

wallet/wallettool.cpp:133:33: warning: loop variable 'warning' of type 'const bilingual_str' creates a copy from type 'const bilingual_str' [-Wrange-loop-analysis] for (const auto warning : warnings) { ^ wallet/wallettool.cpp:133:22: note: use reference type 'const bilingual_str &' to prevent copying for (const auto warning : warnings) { ^~~~~~~~~~~~~~~~~~~~ 1 warning generated.

maflcko · 2020-08-14T13:25:12Z

@ryanofsky Are you going to submit this patch as a pull: #19457 (comment) ?

…extraneous declarations 0e279fe walletdb: Remove unused static functions from walletdb.h (Andrew Chow) 9f536d4 wallettool: Have RecoverDatabaseFile return errors and warnings (Andrew Chow) 06e263a Call RecoverDatabaseFile directly from wallettool (Andrew Chow) Pull request description: Followup to bitcoin#19324 addressing some comments. Removes the `SalvageWallet` function in wallettool and instead directly calls `RecoverDatabaseFile` as suggested in bitcoin#19324 (comment) Removes the `LogPrintf`s and `tfm::format`s in `RecoverDatabaseFile` as noted in bitcoin#19324 (comment) Removes the declarations of `VerifyEnvironment` and `VerifyDatabaseFile` that were forgotten in `walletdb.h` as noted in bitcoin#19324 (comment) ACKs for top commit: meshcollider: Code review ACK 0e279fe ryanofsky: Code review ACK 0e279fe, just dropped last commit Tree-SHA512: ffd01f30536c2eab4bf40ba363c3ea916ecef3c8f0c5262040b40498776ffb00f95240204a40e38415d6931800851d0a3fa63ee91efc1d329b60ac317da0363d

ryanofsky · 2020-08-24T20:08:04Z

@ryanofsky Are you going to submit this patch as a pull: #19457 (comment) ?

Sure, created #19793

Summary: 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 cause the salvage to sometimes fail. This is not needed. This is a backport of [[bitcoin/bitcoin#19457 | core#19457]] [1/3] bitcoin/bitcoin@06e263a Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9639

Summary: Instead of logging or printing these errors and warnings, return them to the caller. This is a backport of [[bitcoin/bitcoin#19457 | core#19457]] [2/3] bitcoin/bitcoin@9f536d4 Depends on D9640 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9641

Summary: VerifyEnvironment and VerifyDatabaseFile were removed in D8617/[[bitcoin/bitcoin#19324 | PR19324]], but their declarations weren't. Remove those. This is a backport of [[bitcoin/bitcoin#19457 | core#19457]] [3/3] bitcoin/bitcoin@0e279fe Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9079

achow101 force-pushed the walletdb-statics-followup branch from e7fcfd5 to e3243b2 Compare July 6, 2020 18:52

DrahtBot added the Wallet label Jul 6, 2020

This was referenced Jul 7, 2020

wallettool: Add dump and createfromdump commands #19137

Merged

wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets #19077

Merged

maflcko reviewed Jul 7, 2020

View reviewed changes

src/wallet/salvage.cpp Outdated Show resolved Hide resolved

This was referenced Jul 7, 2020

multiprocess: Add bitcoin-gui -ipcconnect option #19461

Draft

multiprocess: Add bitcoin-wallet -ipcconnect option #19460

Draft

walletdb: Don't remove database transaction logs and instead error #18907

Merged

achow101 force-pushed the walletdb-statics-followup branch from e3243b2 to 7dcb47b Compare July 15, 2020 16:37

ryanofsky approved these changes Jul 21, 2020

View reviewed changes

maflcko reviewed Jul 22, 2020

View reviewed changes

src/wallet/wallettool.cpp Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Jul 22, 2020

achow101 force-pushed the walletdb-statics-followup branch from 7dcb47b to 21349ec Compare July 22, 2020 16:34

achow101 mentioned this pull request Jul 22, 2020

test: Extend salvage test for wallet tool #19078

Closed

DrahtBot removed the Needs rebase label Jul 22, 2020

ryanofsky reviewed Jul 24, 2020

View reviewed changes

maflcko reviewed Jul 25, 2020

View reviewed changes

src/wallet/salvage.cpp Outdated Show resolved Hide resolved

achow101 added 2 commits July 26, 2020 20:22

wallettool: Have RecoverDatabaseFile return errors and warnings

9f536d4

Instead of logging or printing these errors and warnings, return them to the caller.

walletdb: Remove unused static functions from walletdb.h

0e279fe

VerifyEnvironment and VerifyDatabaseFile were removed, but their declarations weren't. Remove those.

achow101 force-pushed the walletdb-statics-followup branch 2 times, most recently from 80e1772 to fbe8816 Compare July 27, 2020 00:25

DrahtBot mentioned this pull request Jul 29, 2020

Remove wallet.dat path handling from wallet.cpp, rpcwallet.cpp #19619

Merged

ryanofsky approved these changes Aug 12, 2020

View reviewed changes

achow101 force-pushed the walletdb-statics-followup branch from fbe8816 to 0e279fe Compare August 13, 2020 00:12

meshcollider reviewed Aug 13, 2020

View reviewed changes

ryanofsky approved these changes Aug 13, 2020

View reviewed changes

fanquake reviewed Aug 14, 2020

View reviewed changes

maflcko merged commit 30dd562 into bitcoin:master Aug 14, 2020

ryanofsky mentioned this pull request Aug 24, 2020

bitcoin-wallet salvage: Return false instead of asserting when a loaded tx isn't new #19793

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

wallet: Cleanup wallettool salvage and walletdb extraneous declarations #19457

wallet: Cleanup wallettool salvage and walletdb extraneous declarations #19457

Uh oh!

Conversation

achow101 commented Jul 6, 2020

Uh oh!

DrahtBot commented Jul 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 commented Jul 22, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jul 24, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 25, 2020

Choose a reason for hiding this comment

Uh oh!

achow101 Jul 27, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 Jul 27, 2020

Choose a reason for hiding this comment

Uh oh!

ryanofsky Aug 12, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Aug 12, 2020

Choose a reason for hiding this comment

Uh oh!

achow101 commented Aug 13, 2020

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake Aug 14, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko commented Aug 14, 2020

Uh oh!

ryanofsky commented Aug 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

DrahtBot commented Jul 6, 2020 •

edited

Loading

maflcko Jul 27, 2020 •

edited

Loading